Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lowering: Keep track of which :enter correspond to which :leave #51590

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Oct 4, 2023

This should be NFC and is intended to allow the optimizer to delete :enter statements (by replacing them with nothing), without leaving dangling :leaves around. This is accomplished by having leave take (a variable number of) :enter tokens (that are already being used by :pop_exception). The semantics are that a literal nothing or an SSAValue pointing to a nothing statement are ignored, and one exception handler is popped for each remaining argument. The actual value of the token is ignored, except that the verifier asserts that it belongs to an :enter.

Note that we don't need to do the same for :pop_exception, because the token generated by an :enter is semantically only in scope for :pop_exception during its catch block. If we determine the :enter is dead, then its catch block is guaranteed to not be executed and will be deleted wholesale by cfg liveness.

I was considering doing something fancier where :leave is changed back to taking an integer after optimization, but the case where the IR size is bigger after this change (when we are :leaveing many handlers) is fairly rare and likely not worth the additional complexity or time cost to do anything special. If it does show up in size benchmarks, I'd rather give :leave a special, compact encoding.

@Keno Keno requested a review from JeffBezanson October 4, 2023 18:38
@brenhinkeller brenhinkeller added the compiler:lowering Syntax lowering (compiler front end, 2nd stage) label Oct 5, 2023
@Keno Keno force-pushed the kf/loweringerror branch from a491800 to eabb7db Compare October 8, 2023 15:02
@Keno
Copy link
Member Author

Keno commented Oct 8, 2023

@nanosoldier runtests()

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. I assume JuliaInterpreter will need a change to keep working after this. I think it would be best if we could merge that PR first, then this?

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Keno added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Oct 9, 2023
@Keno
Copy link
Member Author

Keno commented Oct 9, 2023

SGTM. I assume JuliaInterpreter will need a change to keep working after this. I think it would be best if we could merge that PR first, then this?

JuliaDebug/JuliaInterpreter.jl#592

This should be NFC and is intended to allow the optimizer to
delete :enter statements (by replacing them with `nothing`),
without leaving dangling `:leave`s around. This is accomplished
by having `leave` take (a variable number of) `:enter` tokens
(that are already being used by `:pop_exception`). The semantics
are that a literal `nothing` or an SSAValue pointing to a `nothing`
statement are ignored, and one exception handler is popped for
each remaining argument. The actual value of the token is ignored,
except that the verifier asserts that it belongs to an `:enter`.

Note that we don't need to do the same for :pop_exception, because
the token generated by an `:enter` is semantically only in scope
for :pop_exception during its catch block. If we determine the
`:enter` is dead, then its catch block is guaranteed to not be
executed and will be deleted wholesale by cfg liveness.

I was considering doing something fancier where :leave is changed
back to taking an integer after optimization, but the case where
the IR size is bigger after this change (when we are `:leave`ing
many handlers) is fairly rare and likely not worth the additional
complexity or time cost to do anything special. If it does show
up in size benchmarks, I'd rather give `:leave` a special, compact
encoding.
@Keno Keno force-pushed the kf/loweringerror branch from eabb7db to 77b4694 Compare October 9, 2023 22:33
aviatesk pushed a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Oct 10, 2023
@Keno
Copy link
Member Author

Keno commented Oct 10, 2023

@nanosoldier runtests(["MRICoilSensitivities", "Dynesty", "GMT", "JumpProblemLibrary", "Widgets", "DifferentiableBackwardEuler", "HydroPowerSimulations", "GeoStatsBase", "SDEProblemLibrary", "IterativeLearningControl", "Test", "MTKHelpers", "MinimallyDisruptiveCurves", "GlobalSensitivity", "Intervals", "InteractiveDynamics", "Attractors", "Sundials", "Pyehtim", "Catalyst", "HOODESolver", "LoopVectorization", "ImageFiltering", "CausalityTools", "HclinicBifurcationKit", "CDGRNs", "MimiRFFSPs", "QuantumAnnealingAnalytics", "FourierSpaces", "ALFA", "EnergyCommunity", "Nonconvex", "PlugFlowReactor", "MPSKit"])

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

@Keno
Copy link
Member Author

Keno commented Oct 10, 2023

This all looks good to go. I'll give @JeffBezanson a few hours to take a look if he wants, otherwise, I'll just merge this.

@JeffBezanson
Copy link
Member

Looks basically good, but I'm a bit worried about having cases where it's required to keep a nothing around in the IR. Would be nice if that was always safe to delete.

@Keno
Copy link
Member Author

Keno commented Oct 10, 2023

Looks basically good, but I'm a bit worried about having cases where it's required to keep a nothing around in the IR. Would be nice if that was always safe to delete.

You're allowed to delete it, you just need to propagate it to the usage side, which is always the case (e.g. for functions that return Const(nothing), which looks the same).

@Keno Keno merged commit 3711749 into master Oct 11, 2023
1 check passed
@Keno Keno deleted the kf/loweringerror branch October 11, 2023 14:41
Keno added a commit that referenced this pull request Oct 12, 2023
This leverages the support from #51590 to delete any try catch block
that is known not to be triggered (either because the try-body is
empty to because we have proven `:nothrow` for all contained
statements).
Keno added a commit that referenced this pull request Oct 14, 2023
This leverages the support from #51590 to delete any try catch block
that is known not to be triggered (either because the try-body is
empty to because we have proven `:nothrow` for all contained
statements).
Keno added a commit that referenced this pull request Oct 15, 2023
This leverages the support from #51590 to delete any try catch block
that is known not to be triggered (either because the try-body is
empty to because we have proven `:nothrow` for all contained
statements).
Keno added a commit that referenced this pull request Oct 15, 2023
This leverages the support from #51590 to delete any try catch block
that is known not to be triggered (either because the try-body is empty
to because we have proven `:nothrow` for all contained statements).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants