Cranelift: fix invalid regalloc constraints on try-call with empty handler list.#10709
Merged
fitzgen merged 1 commit intobytecodealliance:mainfrom May 2, 2025
Merged
Conversation
eff5001 to
9f8753e
Compare
Contributor
|
Please remove the @ mention from the commit message and PR description to avoid pinging me every time someone pushes it to their repo. |
…ndler list.
(Reported by bjorn3 -- thanks!)
We handle edge splitting for regalloc moves in a way that is designed to
split minimally (for compiler performance and codegen quality): we only
split edges that are truly critical, i.e., come from a block with more
than one successor and go to a block with more than one predecessor.
In all other cases, there is always a place for these moves to go: if a
block only has one successor, then edge-moves can go before its jump;
and if a block only has one predecessor, then edge-moves can go at the
beginning of that block (before the blockparams' parallel-move). The
former case -- before the branch -- works because of a restriction that
regalloc2 imposes: branches with only one target (that is, unconditional
branches) cannot have any arguments. Otherwise, those uses would occur
after the edge-moves have already shuffled the register state into a
state suitable for the next block. Violating this constraint leads to a
panic.
With ordinary unconditional branches, this is no problem. With
`try_call`s with at least one handler listed, this is also no problem:
such a terminator is seen as a branch with (at least) two targets by
regalloc, so no edge-moves are placed before it, so it's allowed to have
arguments, such as the arguments to the call itself. However, a
`try_call` with *no* handler clauses, though pathological, appears to
regalloc as an unconditional branch and so should not have any
arguments. The included test-case triggers this issue with such a
`try_call` together with a normal-return target branch that has more
than one incoming edge, forcing the location for the moves into the
`try_call`'s block. (The lack of actual edge-moves doesn't matter -- RA2
performs the check on the IR restriction first.) The result is a panic
at compile time.
This PR fixes the issue by extending a similar fix for `br_table`s
(which can trigger a very similar bug if they have only the default
case, i.e., one target) to `try_call{,_indirect}` as well: the
lowered-block order computation, where edge-splits are determined,
pretends that they always have at least two successors. This ensures
that edges will be split as necessary, satisfying the
no-arguments-to-unconditional-terminators restriction.
9f8753e to
c7c36f8
Compare
Member
Author
|
Sorry about that -- amended the commit and edited the description. |
fitzgen
approved these changes
May 2, 2025
Member
fitzgen
left a comment
There was a problem hiding this comment.
Thanks for the detailed description! Made understanding this easy 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(Reported by bjorn3 -- thanks!)
We handle edge splitting for regalloc moves in a way that is designed to split minimally (for compiler performance and codegen quality): we only split edges that are truly critical, i.e., come from a block with more than one successor and go to a block with more than one predecessor. In all other cases, there is always a place for these moves to go: if a block only has one successor, then edge-moves can go before its jump; and if a block only has one predecessor, then edge-moves can go at the beginning of that block (before the blockparams' parallel-move). The former case -- before the branch -- works because of a restriction that regalloc2 imposes: branches with only one target (that is, unconditional branches) cannot have any arguments. Otherwise, those uses would occur after the edge-moves have already shuffled the register state into a state suitable for the next block. Violating this constraint leads to a panic.
With ordinary unconditional branches, this is no problem. With
try_calls with at least one handler listed, this is also no problem: such a terminator is seen as a branch with (at least) two targets by regalloc, so no edge-moves are placed before it, so it's allowed to have arguments, such as the arguments to the call itself. However, atry_callwith no handler clauses, though pathological, appears to regalloc as an unconditional branch and so should not have any arguments. The included test-case triggers this issue with such atry_calltogether with a normal-return target branch that has more than one incoming edge, forcing the location for the moves into thetry_call's block. (The lack of actual edge-moves doesn't matter -- RA2 performs the check on the IR restriction first.) The result is a panic at compile time.This PR fixes the issue by extending a similar fix for
br_tables (which can trigger a very similar bug if they have only the default case, i.e., one target) totry_call{,_indirect}as well: the lowered-block order computation, where edge-splits are determined, pretends that they always have at least two successors. This ensures that edges will be split as necessary, satisfying the no-arguments-to-unconditional-terminators restriction.