Cranelift: don't lower dead notrap loads - #14114
Conversation
Alias analysis's dead-store elimination removed the dead store's `mem_values`
entry, but left the region's last-store slot naming the instruction it had just
deleted. Leaving the removed-store meant that when we then reprocess the
overwriting store, we keyed its lookup on a removed instruction, found nothing,
and failed to notice that (for example) the overwriting store became idempotent
and could also be removed.
With this commit, each store now records the memory version it displaced, and
eliminating a dead store rolls that version back, so a chain like
v1 = load.i32 region0 v0
store region0 v2, v0 ;; dead
store region0 v1, v0 ;; idempotent once the dead store is gone
collapses in the single pass we actually make, rather than removing only one
link in the chain and requiring that we do N passes to fully clean up a chain of
N dead/idempotent stores. This code pattern the shape fused sync adapters emit
around the `MAY_LEAVE` flag and the relevant disas tests each lose a store as a
result.
Before, we would emit a call to a host function, passing the trap code as a constant argument. Now we emit a `trap <code>` instruction directly. This is a large improvement for our sync adapter disas tests.
If a load is marked `notrap` then 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.
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Seems reasonable to me, but I'm going to tag @cfallin on this for a second set of eyes as well to double-check
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. |
github-actions
Bot
added
cranelift
3 participants
If a load is marked
notrapthen it is not side-effecting and we need not emitit 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 #14113