Conversation
In Binaryen's Vacuum pass, a drop of an if expression was previously only sunk into an arm if the other arm ended in an unreachable. When both arms were concrete, the drop was kept wrapping the if expression, forcing the if to retain a concrete block type (requiring multi-byte type index headers for GC types) and preventing dead-code elimination, vacuuming, and PruneDroppedReturns from optimizing dropped expressions within the branches. Furthermore, visitIf contained legacy logic that hoisted drops back outside of the if if both arms were dropped with identical types, which was detrimental for Wasm GC reference types (where block type indexes exceed 1 byte) and broke optimization cascades across passes. This change: 1. Sinks drops into both arms of concrete if-else expressions in visitDrop. 2. Removes the conflicting drop-hoisting logic in visitIf. 3. Adds unit tests and updates lit tests accordingly.
| curr->ifFalse = right; | ||
| curr->finalize(); | ||
| replaceCurrent(Builder(*getModule()).makeDrop(curr)); | ||
| } |
There was a problem hiding this comment.
For MVP types this still saves one byte, so I'm not sure we want to remove this completely?
There was a problem hiding this comment.
Per your other comment, I asked agent to prototype special casing MVP (re-hoist all isBasic()), it didn't make a meaningful difference.
Agent made following argument while prototyping but I didn't dig to verify claims:
Why MVP expressions survive Vacuum::optimizeDrop—and why hoisting hurts downstream passes
Because all MVP basic types are defaultable (Type::isDefaultable()), Vacuum::optimize and Vacuum::optimizeDrop already eliminate (drop X) for every pure expression, unary/binary arithmetic or comparison wrapper, non-trapping load, local.tee, and poppable block. Consequently, an MVP-typed arm can remain a Drop after Vacuum only when the arm is:
-
Call / CallRef / CallIndirect (11 instances in calcworker_wasm):
DeadArgumentElimination::visitDrop only records info->droppedCalls[call] when Drop directly wraps Call.
Re-hoisting drop out of (if cond (drop (call $f)) (drop (call $g))) into (drop (if (result i32) cond (call $f) (call $g))) couples $f's return type to $g's return type and hides (drop (call $f)) from dae-optimizing.
In calcworker_wasm, re-hoisting MVP Drops (Variant B1) prevented DAE from stripping the unused i32 return value of $725 (because $725 was paired in an if-else in $2119 with $841, whose i32 return value is used elsewhere), which also prevented eliminating a caller thunk ($2772). -
Break (br_if carrying a value) or Block with value-carrying branches (5 instances in calcworker_wasm):
ProblemFinder::visitExpression and MergeBlocks::visitDrop require br_if and block to be directly wrapped in Drop to strip unused branch values from enclosing blocks.
| iff->ifFalse->type.isConcrete()) { | ||
| Builder builder(*getModule()); | ||
| iff->ifTrue = builder.dropIfConcretelyTyped(iff->ifTrue); | ||
| iff->ifFalse = builder.dropIfConcretelyTyped(iff->ifFalse); |
There was a problem hiding this comment.
These can just be makeDrop as we know they are concretely typed.
|
It is perhaps odd to move drops outside of an if only for MVP types, but that seems best for code size? |
This branch has not been deployed
2 participants
In Binaryen's Vacuum pass, a drop of an if expression was previously only sunk into an arm if the other arm ended in an unreachable. When both arms were concrete, the drop was kept wrapping the if expression, forcing the if to retain a concrete block type (requiring multi-byte type index headers for GC types) and preventing dead-code elimination, vacuuming, and PruneDroppedReturns from optimizing dropped expressions within the branches.
Furthermore, visitIf contained logic that hoisted drops back outside of the if if both arms were dropped with identical types, which was detrimental for Wasm GC reference types (where block type indexes exceed 1 byte) and broke optimization cascades across passes.
This change: