cfallin requested fitzgen for a review on PR #14324.
cfallin opened PR #14324 from cfallin:fix-fuzzbug to bytecodealliance:main:
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
Onceon 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.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease review the Bytecode Alliance's AI tool usage policy at
https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.mdPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
cfallin requested wasmtime-compiler-reviewers for a review on PR #14324.
github-actions[bot] added the label cranelift on PR #14324.
github-actions[bot] added the label cranelift:area:machinst on PR #14324.
:thumbs_up: fitzgen submitted PR review.
cfallin updated PR #14324.
cfallin has enabled auto merge for PR #14324.
cfallin added PR #14324 Cranelift: fix multiplicity analysis of multi-def instructions. to the merge queue
:check: cfallin merged PR #14324.
cfallin removed PR #14324 Cranelift: fix multiplicity analysis of multi-def instructions. from the merge queue
Last updated: Sep 20 2026 at 18:08 UTC