compiler: peephole pass with fused and constant opcodes - #403
Conversation
🤖 Automated review — Opus + Sonnet consolidated findingsTwo independent AI reviewers (Opus, Sonnet) reviewed this PR; findings were adversarially verified and the HIGH below was independently reproduced by the orchestrator on this branch before posting. 🔴 HIGH — move elision deletes a write to a register that is live-out of a loop (miscompile)
Minimal reproducer (verified on this branch: peephole on → local function id(x) return x end
local c = 0
for i = 1, 2 do
c = i
local y = id(c)
end
return cMechanism: the liveness scan for a rewrite site inside a loop body follows only the back-edge continuation ( Also reproduced with Fix direction: include the loop-exit continuation in 🟡 LOW
Test-coverage notes
Verified clean by both reviewers: constant folding (operand inlining only — int/float, wrap, metamethod, error-hint fidelity over 44 probes), A fix for the HIGH (and the LOW items above where they belong to this branch) will be pushed to this branch shortly. |
|
Pushed two commits addressing the automated review's HIGH miscompile. |
Codegen lowers naively: a fresh temporary for every intermediate value copied into place, _ENV re-read from the upvalue table before every global access, and every literal materialised into a register. A new Lua.Compiler.Peephole pass runs between codegen and bytecode encoding and cleans that up: 1. move elision (retarget the producer, drop the copy) 2. constant folding into _k opcode variants 3. get_upvalue + get_field/set_field fusion (every global access) 4. unreachable code after an unconditional return/break 5. set_upvalue + get_upvalue round-trip collapse 6. close_upvalues removal in closure-free functions plus a recomputed max_registers, clamped so it can only shrink. Every rewrite is gated on the rewritten temporary being dead, answered by scanning the instructions that can follow — including the loop back edge, spelled out instruction by instruction. reads?/3 defaults to 'reads everything' for unrecognised shapes, and functions containing goto opt out entirely. The eight new opcodes (add_k, subtract_k, multiply_k, less_than_k, less_equal_k, equal_k, get_field_upvalue, set_field_upvalue) have handlers in both engines, so the interpreter and the dispatcher stay in lockstep. Their slow paths bridge to the same metamethod helpers as the register forms, and the folded arithmetic keeps hint_a, so error rendering is byte-identical. fib's body goes from 19 opcodes in 6 registers to 10 in 4; a run_oop-style loop body from 30 instructions to 19. Compile with peephole: false to skip the pass; the new differential test compiles a corpus both ways and compares results, output, and rendered exceptions.
Scanning every register 0..max for a read makes the pass quadratic in the register count. The answer is almost always within a slot or two of the incoming bound, so search from the top and stop at the first hit. Cuts the pass's share of compile time roughly in half on a multi-function chunk (parse_chunk of a ~60-line chunk: +19% -> +13.5%).
The riskiest thing the peephole pass can break is a slow path: a folded _k form has to reach the metamethod bridge with its constant boxed, and a fused upvalue-field form has to reach __index / __newindex on _ENV. Add those to the differential corpus and the error battery, covering __add/__sub/__mul/__lt/__le on a folded literal, string-to-number coercion, int64 wrap, float promotion, and _ENV metatables.
`body_futures/2` modelled a loop body's continuation as the back edge
alone — one more trip around the loop, with whatever follows the loop
reachable only after that trip. An unconditional write near the top of
the body therefore registered as a `:killed` in `scan/4` and ended the
scan before the exit path was ever examined. `dead?/3` then declared the
register dead and `elide_move/3` deleted the `{:move, local, tmp}` that
was its only write. The `escaped` flag modelled `:break` but not normal
condition-false exit or `for` exhaustion, so every loop kind was
affected.
local function id(x) return x end
local c = 0
for i = 1, 2 do
c = i
local y = id(c)
end
return c -- 0 with the pass on, 2 with it off
The back-edge trip is now tagged `{:back_edge, instructions}`. A read
inside it still settles liveness, but a kill inside it settles only the
back-edge path: `dead?/3` continues into the exit continuation, so a
register the code after the loop reads stays live regardless of what the
next iteration would do to it. Cost across the 5.3 suite and fixtures is
1.3% fewer elisions (13414 -> 13233 instructions removed).
Regression tests cover the reproducer plus an arithmetic producer,
`while`, `repeat`, `generic_for`, a method-call argument, and the same
shape nested in a function (the dispatcher path). A randomized
differential property generates small integer programs over three
locals — loop bodies that write them unconditionally, calls that copy
them into arguments, reads after the loop — and asserts the pass off and
on agree; it fails on 300 runs without this fix. Two module comments
that claimed the pass only ever removes reads now state what move
elision actually does to the copy's write.
Claude-Session: https://claude.ai/code/session_01BYHGFLoHBJAsUrTzjVp5nC
`instruction_size({:load_nil, dest, count})` returned `dest + count`, but
the opcode clears `count + 1` registers — `dest..dest + count`, per
`Lua.Compiler.Bytecode` and the dispatcher's `clear_nils(regs, dest,
count + 1)`. It undercounted the frame by one slot. Nothing emits
`:load_nil` today, but `Peephole.recompute_max_registers/3` uses
`instruction_peak/1` as the floor it may shrink to, so the undercount
would become a too-narrow register file rather than a benign slack.
`Peephole`'s own `writes?/2` clause already used the correct extent.
Claude-Session: https://claude.ai/code/session_01BYHGFLoHBJAsUrTzjVp5nC
The close-upvalue watermark is a scope-analysis property. This chunk creates no closures, so the peephole pass drops the `close_upvalues` opcodes the assertion reads it through — correctly, but that leaves nothing to observe. Compile with the pass off, matching the sibling test that covers the same property for parsed source. Claude-Session: https://claude.ai/code/session_01BYHGFLoHBJAsUrTzjVp5nC
0911b53 to
4a8fc63
Compare
1 participant
Codegen lowers naively — a fresh temporary for every intermediate value copied
into place,
_ENVre-read out of the upvalue table before every global access,every literal materialised into a register. That is the right call for codegen;
it just leaves a pile of purely local rewrites on the floor. This adds
Lua.Compiler.Peephole, which runs between codegen and bytecode encoding andpicks them up.
Rules
into its real home writes the real home directly, and the copy disappears.
The copy is searched for within a bounded window (16 instructions) rather
than requiring strict adjacency —
move base+1, tmp1; move base+2, tmp2separates every call argument and every
forheader from its producer.load_constant K, kfeeding an arithmetic orcomparison op as its right operand folds into a
_kvariant carrying theliteral inline:
add_k,subtract_k,multiply_k,less_than_k,less_equal_k,equal_k. The register that held the constant goes awaywith it.
get_upvalue t, i+get_field d, t, name→get_field_upvalue d, i, name, and theset_fieldmirror. Every globalread and write inside a function is exactly this pair, so this halves the
instruction count of all of them.
return/breakis dropped —if n < 2 then return n endcarries aclose_upvaluescodegen appendsunconditionally.
set_upvalue i, rimmediately followed byget_upvalue d, ibecomesmove d, r; the write stays.close_upvalues. A function that builds no closures can neverhave an open cell over one of its own registers, so it has nothing to close.
The gate is the whole function, deliberately:
gotocloses at explicitlevels and loop bodies close at iteration boundaries, and neither is safe to
reason about block by block.
max_registersre-derived from the rewritten stream — this is where agood chunk of the win is, since every call frame allocates that tuple and
every
setelementcopies it. Clamped to the incoming value so it can onlyever shrink, and bounded below by the highest index the rewritten stream
still touches.
Register compaction (renumbering to close the gaps elision leaves) is not
here; recomputing the peak captures most of it and compaction is a much bigger
change.
fib, before and after
Before — 19 opcodes,
max_registers = 6:After — 10 opcodes,
max_registers = 4:A
run_oop-style loop body goes from 30 instructions to 19. An emptyforheader goes from 5 instructions to 3.
Both engines
Lua.VM.Executor(which runs the main chunk, and every prototype the encoderrejects) and
Lua.VM.Dispatcher(which runs everything else) both execute therewritten stream, so all eight new opcodes have handlers in both. Their slow
paths bridge to the same
dispatcher_binop/dispatcher_cmp/index_value/table_newindexhelpers as the register forms, with theconstant boxed, so
__add/__sub/__mul/__lt/__le/__eq/__index/__newindexfidelity is unchanged. The folded arithmetic keepshint_a; the constant side never carried a hint to begin with, so(local 'n')-style error suffixes render identically.Safety
Every rewrite is gated on the rewritten temporary being dead — never read again
on any path that can follow. Liveness is answered by scanning the instructions
that can execute after the rewrite site: the rest of the enclosing block, then
each enclosing block outward, plus one more trip around any enclosing loop
spelled out instruction by instruction (so a temporary rewritten at the top of
every iteration is correctly dead at the bottom of the previous one — that back
edge is what takes the oop loop body from 21 down to 19). A
breakreachedbefore a killing write suppresses the kill.
reads?/3defaults to "reads everything" for a shape it does not recognise, soan opcode added to codegen without a clause here disables the optimisation
rather than miscompiling it.
:closurecounts as reading every parent registerits child prototype captures. Functions containing
goto/::label::opt outentirely.
Validation
mix format,mix compile --warnings-as-errors,mix dialyzer— clean.mix test --include slow --include lua53 --include differential— 2771passed, 16 skipped. Website suite: 54 passed.
test/lua/compiler/peephole_test.exs): a 48-programcorpus plus the runnable fixtures compiled twice —
peephole: falsevs on —evaluated both ways, comparing return values and captured stdout. An
18-program failing battery compares
Lua.format_exception/1output byte forbyte.
test/lua53_tests/*.luafile is compiled bothways and each prototype asserted to satisfy
max_registers(after) <= max_registers(before),instruction_peak(after) <= instruction_peak(before),instruction count non-increasing, and
max_registers >= instruction_peak(theperf(vm): drop +16 register buffer on interpreter :call path #312 backstop invariant).
_kform where the right operand is a register,none for a literal on the left, none for ops without a
_kvariant(
/,%,^); no upvalue fusion in the chunk (its_ENVlives in aregister, not an upvalue); a
goto-carrying function's instruction stream isbyte-identical to codegen's.
peephole: falseis a real compile option, not test-only scaffolding — it iswhat makes the differential meaningful.
Measured
MIX_ENV=prod, min-of-11-rounds × 40 reps, alternating A/B across 6interleaved passes, against the merge base (
perf/vm-call-path+perf/compiler-front). µs per iteration.fib(15)chunkoop(50)(benchmarks/oop.exsshape)closures(100)(benchmarks/closures.exsshape)Lua.parse_chunk, small snippetLua.parse_chunk, ~60-line chunkRound-to-round spread was tight on both sides (e.g.
fibbase 447–465, new319–329), so the separation is well outside the noise. The pass costs ~1.4 µs
on a small snippet and ~20 µs on a three-function chunk — paid once per
compile, against a 1.09–1.40× win on every execution.
Caveat: the machine had concurrent load throughout. The alternating A/B and
min-of-N are there to absorb it, and the tight per-round spreads suggest they
did, but treat the absolute numbers as indicative rather than publishable.
Also
website: the disassembler and the/reference/opcodespage learned theeight new opcodes (they degraded gracefully via the generic formatter, but
the reference should not be silently incomplete). New "Fused forms" category.
test/lua/vm/upvalue_test.exs: the sibling-empty-loop-body threshold test nowcompiles with
peephole: false. It pins a scope-analysis property observedthrough
close_upvaluesopcodes that rule 6 correctly removes from thatclosure-free chunk.