diff --git a/issues/ISS-403.md b/issues/ISS-403.md new file mode 100644 index 00000000..08d9815b --- /dev/null +++ b/issues/ISS-403.md @@ -0,0 +1,233 @@ +# ISS-403: CSE merges repeated constants into long live ranges that nothing rematerializes + +## Metadata +- Type: bug +- Status: open +- Priority: 2 +- Labels: regalloc, codegen, optimization, code-size, agent +- Assignee: unassigned +- Created: 2026-08-07 +- Updated: 2026-08-07 +- External ref: https://github.com/Milky2018/wasmoon/issues/486 + +## Description + +Emitted code doubles in size once a function reuses integer constants. +The extra bytes are entirely spill and reload traffic. + +Marginal cost per wasm op, sweeping function size with a fixture that +cycles through 256 distinct addresses: + +| ops | marginal B/op | +| ---: | ---: | +| 25–250 | 16.0 | +| 280 | 16.4 | +| 300 | 32.0 | +| 350 | 32.0 | +| 20,000 | 33.9 | + +A sharp doubling between 280 and 300 ops, then flat. 16 B/op is the +correct lowering for `mem[a] = mem[a] + k`: a load, an add, a store, and +one address instruction. + +The cliff is not about size. It sits exactly where the fixture starts +*reusing* address constants. Rerunning with every address distinct: + +| ops | B/op | +| ---: | ---: | +| 400, 256 distinct addresses | 32.0 | +| 400, all distinct | 16.1 | +| 800, all distinct | 16.0 | + +No cliff at all. The variable is whether constants repeat, not how many +there are. + +## Root cause + +CSE merges the repeated `iconst`s, and nothing afterward undoes it. + +At 350 ops with 256 distinct addresses there are 700 address uses and +264 `iconst` instructions after O2 — merged. At 400 ops all-distinct +there are 408, with nothing to merge. + +Deduplicating a repeated constant replaces N short live ranges with one +live range spanning first use to last use, here the whole function. With +256 such constants live at once against roughly 30 allocatable registers, +the allocator gives each its own stack slot and reloads it at every use. + +Decoding the 678,316 bytes emitted for a 20,000-op function: + +| | share | +| --- | ---: | +| reload from stack | 42.9% | +| spill to stack | 21.6% | +| real memory ops | 23.6% | +| arithmetic | 11.8% | + +**64.5% of the emitted code is stack traffic**, spread over 244 distinct +stack slots at about 159 accesses each. + +The same signature appears in the module reported in GitHub #486: 1,027 +spill slots, 829 spills, 48,896 reloads. A 59:1 reload-to-spill ratio is +what "spilled once, reloaded everywhere" looks like; genuine pressure +from short-lived intermediates does not produce it. + +So CSE is doing exactly what it is designed to do, and for constants that +is a pessimization. + +## Design + +The missing piece is rematerialization: rather than keep one definition +alive across the whole function and spill it, rebuild a cheap pure value +where it is used. For an integer constant that is one `movz`, with no +stack slot and no memory traffic. + +The codebase already holds this idea and applies it to exactly one +operand class. `EnvironmentField` values are rematerialized during +AArch64 lowering — see the `AArch64 lowering rematerializes stable fields +under integer pressure` test — and `opt_passes_cse_gvn.mbt` deliberately +skips `GlobalValue(ContextField(_, Stable, _))` so the IR does *not* merge +them into one long-lived value. Those two decisions are one coherent +policy. + +Constants get the opposite treatment on both halves: the IR merges them, +and nothing remats them. That is the worst of the two combinations, and +the inconsistency is the actual defect. + +### What Cranelift does + +Worth stating up front, because it corrects an assumption this issue was +first written with: **regalloc2 has no rematerialization at all.** There +are zero occurrences of the concept in its source. The register allocator +is not where this is solved. + +Cranelift solves it during **e-graph elaboration**, and splits policy from +mechanism: + +- Policy lives in rewrite rules, `codegen/src/opts/remat.isle`. The set is + small and deliberate: `iconst`, `f32const`, `f64const`, `bnot`, and the + ALU-with-one-constant-operand forms of `iadd`/`isub`/`band`/`bor`/`bxor`. + Its own comment gives the criterion — these are "neutral (add-with-imm) + or positive (iconst) for register pressure, and these ops are very + cheap". A rule marks a value with `(remat x)`, which adds it to a set. +- Mechanism is `maybe_remat_arg` in `codegen/src/egraph/elaborate.rs`. + When an argument's defining block differs from the block it is being + used in and the value is marked, elaboration clones the defining + instruction in before the use and rewrites the argument to the clone. + +Two details of that mechanism matter: + +- The copy is memoised on `(insert_block, value)`, so the granularity is + **once per block, not once per use**. Inside a block one register serves + every use; across blocks the live range would otherwise stretch. That is + the middle ground between full CSE and full duplication. +- It does not recurse into the rematerialized instruction's own arguments, + explicitly to avoid needing a second fixpoint loop (their TODO #7313). + +The placement decision is unified with LICM in the same pass: a pure value +is hoisted out of a loop when all its arguments are loop-invariant, and +kept at the use otherwise. There is a pointed exception for pure values +with no arguments, which are allowed to hoist at most one loop level +rather than to the function entry, and the stated reason is avoiding +"too much register pressure on the entire function". That is this issue's +failure mode, named and guarded against in their design. + +### What that means here + +Wasmoon already has the structure this needs. `class_to_value` is keyed by +e-class and walked over the dominator tree with entries undone on exit, so +reuse is already dominance-scoped exactly as Cranelift's `ScopedHashMap` +is. What is missing is only the marked set and the clone-into-the-use-block +step. + +So the design is: + +1. Mark cheap pure values during rewriting — constants first, then the + ALU-with-immediate forms — into a remat set. +2. In `collect_elaboration_steps`, where a dominating value is currently + returned as `Existing(value)` for free, return it only when the value + is defined in the block being elaborated or is not marked. Otherwise + plan a fresh build, memoised per block so repeated uses in one block + still share. +3. Extend the same treatment to `EnvironmentField`, which is currently + rematerialized separately in AArch64 lowering, so the two collapse into + one target-independent mechanism. + +Doing this in elaboration rather than in the register allocator is not +just following Cranelift. For the allocator to rematerialize, it would +have to be able to emit target instructions for each value it declines to +spill, which forces the policy back down into every target's lowering — +which is exactly the duplication that made `EnvironmentField` a special +case in the first place. + +Address folding is a smaller adjacent win worth doing in the same pass +over this code: `ScalarLoad`/`ScalarStore` carry a displacement field that +is left at 0 while `IntBinaryImmediate(W64, Add, 0)` and +`IntBinaryImmediate(W64, Add, 4)` compute the address into a register. +Folding the constant offset into the addressing mode removes those +instructions and the intermediate values that go with them. + +Address folding is a smaller adjacent win worth doing in the same pass +over this code: `ScalarLoad`/`ScalarStore` carry a displacement field that +is left at 0 while `IntBinaryImmediate(W64, Add, 0)` and +`IntBinaryImmediate(W64, Add, 4)` compute the address into a register. +Folding the constant offset into the addressing mode removes those +instructions and the intermediate values that go with them. + +## Acceptance Criteria + +- [ ] Cheap pure values marked for remat are rebuilt once per using block + instead of being reused across blocks and spilled. +- [ ] Integer constants and `EnvironmentField` values go through one + target-independent mechanism rather than two. +- [ ] The 256-distinct-address fixture stays at 16 B/op at 400 ops and + beyond, with no cliff. +- [ ] Constant offsets fold into load/store displacements. +- [ ] Workspace tests and the WAST corpus pass in both modes. + +## Relationships +- Depends on: none +- Parent: none +- Related: ISS-402, ISS-371 +- Discovered from: GitHub #486 + +## Notes + +- 2026-08-07: Found while instrumenting the MilkIR-to-VCode expansion + that GitHub #486 reports as 8,632 instructions becoming 225,372. + + Two hypotheses were tested and disproved before this one, both worth + recording so they are not retried. First, that GVN skipping `Stable` + globals was an inverted condition: removing the skip does cut redundant + `global_value` in the IR, 7 down to 2 on a small function, but changes + the emitted machine code for 5,000-op and 20,000-op functions by zero + bytes — 678,316 before and after, byte for byte. It changes nothing + because backend lowering already remats those values, which is what + makes the skip deliberate rather than a bug. Second, that the fixed + 300-instruction GVN work budget starves large functions: raising it to + the full function size also changed the output by zero bytes. + + Both are real inefficiencies. Neither drives code size, and the measured + cause turned out to be the reverse of the first hypothesis — not too + little CSE, but CSE without rematerialization. + +- 2026-08-07: Scope not established. This is verified for integer + constants in a synthetic fixture, where it accounts for a factor of 2. + The function in GitHub #486 also contains `call_indirect` and + `struct.new`, measured separately at 7.7x and 5.4x expansion, so remat + should not be assumed to account for the full 26x reported there. + +- 2026-08-07: Read Cranelift before designing this, which corrected the + starting assumption. This issue was first written claiming regalloc2 + rematerializes; it does not, and the concept does not appear in its + source at all. Cranelift does it in e-graph elaboration, with the policy + in `remat.isle` rules and the mechanism in `elaborate.rs`. The design + section above is written from that reading rather than from the earlier + guess. + + The useful discovery is how little wasmoon is missing. `class_to_value` + is already dominator-scoped with entries undone on exit, which is the + same structure as Cranelift's `ScopedHashMap`. The gap is the marked set + and the per-block rebuild, not the surrounding machinery. + +## Close Notes diff --git a/issues/README.md b/issues/README.md index 736efece..9e9c7d01 100644 --- a/issues/README.md +++ b/issues/README.md @@ -8,6 +8,7 @@ Generated by derive-tracker.wasm | --- | ---: | --- | --- | --- | --- | | [ISS-402](ISS-402.md) | 1 | bug | unassigned | The AArch64 branch-range fallback is not on the path the JIT compiles through | aarch64, emission, jit, robustness, agent | | [ISS-371](ISS-371.md) | 2 | task | unassigned | Reduce backtracking-allocator compile time on large functions | regalloc, compile-time, performance, agent | +| [ISS-403](ISS-403.md) | 2 | bug | unassigned | CSE merges repeated constants into long live ranges that nothing rematerializes | regalloc, codegen, optimization, code-size, agent | | [ISS-390](ISS-390.md) | 3 | task | unassigned | Rebuild sanitizer coverage as a dedicated package, not a compiler shim | ci, sanitizers, build-config, agent | | [ISS-399](ISS-399.md) | 3 | refactor | unassigned | `regalloc/planning` is a 2129-line package that only its tests use | regalloc, dead-code, agent | | [ISS-389](ISS-389.md) | 4 | chore | unassigned | `wasmoon/sanitizer_testsuite` is named after a gate that no longer exists | cleanup, testing, agent | @@ -19,6 +20,7 @@ Generated by derive-tracker.wasm | [ISS-402](ISS-402.md) | open | 1 | bug | unassigned | none | none | The AArch64 branch-range fallback is not on the path the JIT compiles through | | [ISS-367](ISS-367.md) | deferred | 2 | bug | unassigned | none | none | macOS UBSan aborts on a negative signal number from moonbitlang/async | | [ISS-371](ISS-371.md) | open | 2 | task | unassigned | none | none | Reduce backtracking-allocator compile time on large functions | +| [ISS-403](ISS-403.md) | open | 2 | bug | unassigned | none | none | CSE merges repeated constants into long live ranges that nothing rematerializes | | [ISS-390](ISS-390.md) | open | 3 | task | unassigned | none | none | Rebuild sanitizer coverage as a dedicated package, not a compiler shim | | [ISS-399](ISS-399.md) | open | 3 | refactor | unassigned | none | none | `regalloc/planning` is a 2129-line package that only its tests use | | [ISS-177](ISS-177.md) | deferred | 4 | bug | unassigned | none | none | Implement a Windows fd readiness backend for poll_oneoff | @@ -425,6 +427,7 @@ graph TD ISS_400["ISS-400: A v128 exception payload makes the JIT refuse the whole module"] ISS_401["ISS-401: Graph walks over input-sized structures still recurse on the native stack"] ISS_402["ISS-402: The AArch64 branch-range fallback is not on the path the JIT compiles through"] + ISS_403["ISS-403: CSE merges repeated constants into long live ranges that nothing rematerializes"] ISS_002 --> ISS_003 ISS_002 --> ISS_004 ISS_003 --> ISS_005