Wasmtime GitHub notifications bot (Aug 10 2026 at 20:50): Wasmtime GitHub notifications bot (Aug 10 2026 at 20:50):fitzgen opened PR #14114 from fitzgen:do-not-lower-dead-notrap-loads to bytecodealliance:main:
If a load is marked
notrapthen it is not side-effecting and we need not emit
it when its loaded value is unused.Note that we do still have to increment the side effect color for these
instructions to prevent merging/sinking loads across stores that could change
the value they observe.This drops two dead vmctx flag loads from our component-model fused adapter's
fast path.Depends on https://github.com/bytecodealliance/wasmtime/pull/14113
Wasmtime GitHub notifications bot (Aug 10 2026 at 20:50):fitzgen requested alexcrichton for a review on PR #14114.
Wasmtime GitHub notifications bot (Aug 10 2026 at 20:50):fitzgen requested wasmtime-compiler-reviewers for a review on PR #14114.
Wasmtime GitHub notifications bot (Aug 10 2026 at 20:50):fitzgen requested wasmtime-core-reviewers for a review on PR #14114.
Wasmtime GitHub notifications bot (Aug 10 2026 at 21:21)::thumbs_up: alexcrichton submitted PR review:
Seems reasonable to me, but I'm going to tag @cfallin on this for a second set of eyes as well to double-check
Wasmtime GitHub notifications bot (Aug 10 2026 at 21:21):alexcrichton requested cfallin for a review on PR #14114.
Wasmtime GitHub notifications bot (Aug 10 2026 at 22:45):cfallin commented on PR #14114:
Note that we do still have to increment the side effect color for these
instructions to prevent merging/sinking loads across stores that could change
the value they observe.I may have missed it but I don't see a test that explains this case -- could you sketch it out a bit more?
I ask because IMHO, a load that does not trap and whose result is unused is effectively... effect-free (so to speak), so it seems sound to me to not bump the InstColor. Stores will of course still increment the color, so the "across stores" bit of your note is still protected against regardless. But a load should be sinkable across a dead, non-trapping load (reordering loads won't change their results, and if the one reordered-over doesn't trap, then the trapping location can't change); and color is what controls merging loads-into-consumers. So it seems simpler to eliminate the special case here.
Wasmtime GitHub notifications bot (Aug 10 2026 at 23:53):github-actions[bot] added the label cranelift on PR #14114.
Wasmtime GitHub notifications bot (Aug 10 2026 at 23:53):github-actions[bot] added the label cranelift:area:machinst on PR #14114.
Wasmtime GitHub notifications bot (Aug 13 2026 at 16:59):fitzgen commented on PR #14114:
I may have missed it but I don't see a test that explains this case -- could you sketch it out a bit more?
I guess it isn't that we need to color dead
notraploads, its that when we are coloring in the pre-pass insideLower::new, we don't have the use counts yet, and live loads require coloring to avoid sinking them across stores that could change the memory location they observe, so we have to conservatively color all loads at that point in time. And therefore we need a different inst predicate for deciding whether to increment coloring vs whether to lower an instruction.This behavior is covered by existing filetests, which start panicking in load sinking if we e.g. don't increment the color for loads.
Does that clear things up for you?
Wasmtime GitHub notifications bot (Aug 13 2026 at 17:00):fitzgen edited a comment on PR #14114:
I may have missed it but I don't see a test that explains this case -- could you sketch it out a bit more?
I guess it isn't that we need to color dead
notraploads, its that when we are coloring in the pre-pass insideLower::new, we don't have the use counts yet, and live loads require coloring to avoid sinking them across stores that could change the memory location they observe, so we have to conservatively color all loads at that point in time. And therefore we need a different inst predicate for deciding whether to increment coloring vs whether to lower an instruction even if it doesn't have uses.This behavior is covered by existing filetests, which start panicking in load sinking if we e.g. don't increment the color for loads.
Does that clear things up for you?
Wasmtime GitHub notifications bot (Aug 13 2026 at 17:22):cfallin commented on PR #14114:
Ah, OK, that makes sense. Your wording read to me as if we were explicitly deciding to keep coloring even for dead loads, but that's not the case; coloring happens in a prepass. (Wrote this code six years ago and have forgotten many of the details; sorry!)
Wasmtime GitHub notifications bot (Aug 13 2026 at 18:35):fitzgen updated PR #14114.
Wasmtime GitHub notifications bot (Aug 13 2026 at 18:46):fitzgen updated PR #14114.
Wasmtime GitHub notifications bot (Aug 13 2026 at 19:12):fitzgen added PR #14114 Cranelift: don't lower dead notrap loads to the merge queue.
Wasmtime GitHub notifications bot (Aug 13 2026 at 19:39)::check: fitzgen merged PR #14114.
Wasmtime GitHub notifications bot (Aug 13 2026 at 19:39):fitzgen removed PR #14114 Cranelift: don't lower dead notrap loads from the merge queue.
Last updated: Aug 30 2026 at 10:08 UTC