Cranelift: fix uadd_overflow + load sinking. - #14272
Merged
Merged
Conversation
In bytecodealliance#14271 we got a fuzzbug that had a `uadd_overflow` with (i) a sinkable load as one argument, (ii) multiple uses of the flag output, so its flag result would have to be materialized anyway. This results in the lowering happening twice: once to feed the flags into the branch, and once to actually materialize the bool value. That's fine: the whole point of the new mechanism is that the common case is to use the overflow as a branch input only, not as a materialized bool, so a double lowering here is harmless (two extra cycles). The issue arises because of a bit of logic I had forgotten we added several years ago in bytecodealliance#9510 that declares *all* multi-def instructions as "value roots" that will only be lowered *once*. This allows loads to sink into such instructions always (if only used once by that instruction of course), but in turn requires a promise that we will do what we say on the tin: we will only *ever* lower any multi-def instruction once. I believe that this eliminates any practical way of optimizing overflow-flag insts, or doing fusion of bool flags into conditional branches at all, because these inherently require lowering at use sites (because of the way that we don't regalloc flags). It is also a somewhat dangerous (IMHO, now with perspective) exception to our otherwise principled "multiplicity" analysis: we otherwise assume (i) that any given instruction is only lowered once, unless (ii) truly used multiple times in the DFG. That is what allows us to reason about code motion of loads in a princpled way (because we can't duplicate a load). That principle is pretty simple; declaring some instructions "roots" and allowing them to "kill" multiplicity adds this footgun that will strike whenever we forget the exception and write a lowering rule like the overflow cases. Fortunately it seems we don't actually get any test failures when removing that feature (and the test from bytecodealliance#9510 is still in-tree?), so it's not required anymore; so this PR removes the feature. The attached test will panic without the fix. Fixes bytecodealliance#14271. (Some more philosophical thought: our pre-pass that computes multiplicity is itself a choice, but the alternative requires us to give up single-pass lowering altogether and forces code motion into an iterative/fixpoint kind of framework. Consider: we have multiple uses of a given load; when we see the first use, how do we know whether it's the only use (and we can sink it into here) or there will be another? The multiplicity analysis is what answers that ahead of time, and despite its main limitation (it cannot be updated live), it seems to work well overall if we stick to the framework.)
fitzgen
approved these changes
Sep 3, 2026
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.
In #14271 we got a fuzzbug that had a
uadd_overflowwith (i) a sinkable load as one argument, (ii) multiple uses of the flag output, so its flag result would have to be materialized anyway.This results in the lowering happening twice: once to feed the flags into the branch, and once to actually materialize the bool value. That's fine: the whole point of the new mechanism is that the common case is to use the overflow as a branch input only, not as a materialized bool, so a double lowering here is harmless (two extra cycles).
The issue arises because of a bit of logic I had forgotten we added several years ago in #9510 that declares all multi-def instructions as "value roots" that will only be lowered once. This allows loads to sink into such instructions always (if only used once by that instruction of course), but in turn requires a promise that we will do what we say on the tin: we will only ever lower any multi-def instruction once.
I believe that this eliminates any practical way of optimizing overflow-flag insts, or doing fusion of bool flags into conditional branches at all, because these inherently require lowering at use sites (because of the way that we don't regalloc flags).
It is also a somewhat dangerous (IMHO, now with perspective) exception to our otherwise principled "multiplicity" analysis: we otherwise assume (i) that any given instruction is only lowered once, unless (ii) truly used multiple times in the DFG. That is what allows us to reason about code motion of loads in a princpled way (because we can't duplicate a load). That principle is pretty simple; declaring some instructions "roots" and allowing them to "kill" multiplicity adds this footgun that will strike whenever we forget the exception and write a lowering rule like the overflow cases.
Fortunately it seems we don't actually get any test failures when removing that feature (and the test from #9510 is still in-tree?), so it's not required anymore; so this PR removes the feature. The attached test will panic without the fix.
Fixes #14271.
(Some more philosophical thought: our pre-pass that computes multiplicity is itself a choice, but the alternative requires us to give up single-pass lowering altogether and forces code motion into an iterative/fixpoint kind of framework. Consider: we have multiple uses of a given load; when we see the first use, how do we know whether it's the only use (and we can sink it into here) or there will be another? The multiplicity analysis is what answers that ahead of time, and despite its main limitation (it cannot be updated live), it seems to work well overall if we stick to the framework.)