Skip to content

Cranelift: fix uadd_overflow + load sinking. - #14272

Merged
fitzgen merged 2 commits into
bytecodealliance:mainfrom
cfallin:fuzzbug-fix
Sep 3, 2026
Merged

Cranelift: fix uadd_overflow + load sinking.#14272
fitzgen merged 2 commits into
bytecodealliance:mainfrom
cfallin:fuzzbug-fix

Conversation

@cfallin

@cfallin cfallin commented Sep 2, 2026

Copy link
Copy Markdown
Member

In #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 #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.)

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.)
@cfallin
cfallin requested a review from fitzgen September 2, 2026 23:05
@cfallin
cfallin requested a review from a team as a code owner September 2, 2026 23:05
@github-actions github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Sep 3, 2026
@fitzgen
fitzgen added this pull request to the merge queue Sep 3, 2026
Merged via the queue into bytecodealliance:main with commit 84dbb9f Sep 3, 2026
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic in lowering: self.value_lowered_uses[*result] == 0

2 participants