Skip to content

Cranelift: fix multiplicity analysis of multi-def instructions. - #14324

Open
cfallin wants to merge 1 commit into
bytecodealliance:mainfrom
cfallin:fix-fuzzbug
Open

Cranelift: fix multiplicity analysis of multi-def instructions.#14324
cfallin wants to merge 1 commit into
bytecodealliance:mainfrom
cfallin:fix-fuzzbug

Conversation

@cfallin

@cfallin cfallin commented Sep 12, 2026

Copy link
Copy Markdown
Member

This is a follow-up that actually finishes the work of #14272 and fixes #14317 as a result.

It turns out that I had not connected the dots that instructions are the unit onto which multiplicity should attach, not values, because multiplicity is all about how many times a given computation can be lowered. (And then preventing sinking operations that can only happen once, like loads, in contexts with multiplicity.) Use of any output of an instruction can cause that instruction to be lowered; counting multiplicity of Once on each of its outputs (and deciding lowering accordingly) is not correct the instruction could be lowered once for each of those separate uses and then its args might merge in and be lowered more than once.

This does regress the load-sinking-into-i128-ops that #9510 originally aimed to enable. (I had suspected it should have regressed earlier but failed to dig all the way on that hunch; the above inst-vs-value issue hid it.)

I am positing that that regression is something we should accept: a clear mental model is more important. And the problem is fundamentally an NP-hard tiling problem (to see why, observe that there is no optimal substructure: earlier lowering decisions change whether later lowerings actually happen more than once, so earlier suboptimal lowerings could enable later better lowerings/sinkings or vice versa). The "roots" thing was a bandaid that was choosing one particular greedy approach to the NP-hard problem. This PR and #14272 together go back to the more direct greedy approach that is easier to articulate: (i) we compute how many times an instruction could be lowered, in the worst case, if isel on any uses matches multiple levels deep; (ii) we don't ever permit isel to combine potentially-multiple-lowered operands when they have side-effects.

(If we decide otherwise, and want to keep i128+load sinking, I believe we have to delete all of our uadd/umul-overflow lowerings that combine flags, because those fundamentally can lower a multi-def inst more than once, which goes against the whole premise of #9510. We just have to always materialize flags in that case. And per earlier discussions, peephole can't solve it (because elab can put other insts "in the middle") so we have no other solution to that problem.)

Fixes #14317.

This is a follow-up that actually finishes the work of bytecodealliance#14272 and
fixes bytecodealliance#14317 as a result.

It turns out that I had not connected the dots that *instructions* are
the unit onto which multiplicity should attach, not values, because
multiplicity is all about how many times a given computation can be
lowered. (And then preventing sinking operations that can only happen
once, like loads, in contexts with multiplicity.) Use of any output of
an instruction can cause that instruction to be lowered; counting
multiplicity of `Once` on each of its outputs (and deciding lowering
accordingly) is not correct the instruction could be lowered once
for *each* of those separate uses and then *its* args might merge in
and be lowered more than once.

This does regress the load-sinking-into-i128-ops that bytecodealliance#9510 originally
aimed to enable. (I had suspected it should have regressed earlier but
failed to dig all the way on that hunch; the above inst-vs-value issue
hid it.)

I am positing that that regression is something we should accept: a
clear mental model is more important. And the problem is fundamentally
an NP-hard  tiling problem (to see why, observe that there is no
optimal substructure: earlier lowering decisions change whether later
lowerings *actually* happen more than once, so earlier suboptimal
lowerings could enable later better lowerings/sinkings or vice
versa). The "roots" thing was a bandaid that was choosing one
particular greedy approach to the NP-hard problem. This PR and bytecodealliance#14272
together go back to the more direct greedy approach that is easier to
articulate: (i) we compute how many times an instruction *could* be
lowered, in the worst case, if isel on any uses matches multiple
levels deep; (ii) we don't ever permit isel to combine
potentially-multiple-lowered operands when they have side-effects.

(If we decide otherwise, and want to keep i128+load sinking, I believe
we have to delete all of our uadd/umul-overflow lowerings that combine
flags, because those fundamentally can lower a multi-def inst more
than once, which goes against the whole premise of bytecodealliance#9510. We just have
to always materialize flags in that case. And per earlier discussions,
peephole can't solve it (because elab can put other insts "in the
middle") so we have no other solution to that problem.)

Fixes bytecodealliance#14317.
@cfallin
cfallin requested a review from fitzgen September 12, 2026 22:38
@cfallin
cfallin requested a review from a team as a code owner September 12, 2026 22:38
@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 12, 2026
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.

assertion failed: self.value_lowered_uses[*result] == 0

1 participant