jit(cranelift): three ref-root/demoted-home GC fixes, and the def-site store removal - #1345
Conversation
Twelve backend arms stored a freshly defined Ref result into that raw's ref-root home right after `def_var`: SameAsR, the Call/CallPure/CallLoopinvariant Ref arm, the CallAssembler merge, GcLoadR, GcLoadIndexedR, VirtualRefR, New/NewWithVtable in both its GC and no-GC forms, NewArray/NewArrayClear, GetarrayitemRawR, ThreadlocalrefGet, and Newstr/Newunicode. No reader misses them. A demoted raw's definition always precedes the LABEL that demotes it, because `compute_loop_phi_keep` refuses to demote a raw redefined after the LABEL; that LABEL's fall-through seeding writes the same word with the same value immediately before recording the demotion, so the eager store is subsumed. Ahead of the seeding the raw is not demoted, so it follows the ordinary discipline: `spill_ref_roots` over the live slot list precedes every `emit_push_gcmap` under the predicate `get_gcmap` applies, and `get_gcmap`'s unconditional demoted mark cannot fire before the seeding store because the map is filled during emission in op order. Guard maps name dense fail-arg slots only. The remaining escape, `stale_ref_vars`, has no insertion anywhere in the workspace. `LoadFromGcTable`, `CallMallocNursery`, `GetfieldGcR`, `GetarrayitemGcR` and `CondCallValueR` are Ref-result definitions that already did not sync a home, so this makes the arms consistent rather than introducing a new rule. This is not a measurable speedup. fannkuch and nbody A/Bs sit inside the noise band — 34 of 60 paired rounds — even though the emitted store count drops (fannkuch 5609 to 5358, nbody 2877 to 2733, spectral_norm 892 to 842). The stores were unobservable work, not hot work. `cargo test -p majit-backend-cranelift` and `pyre/check.py --backend cranelift` (440/440) pass. Assisted-by: Claude
…easing it `force_token_to_dead_frame` built its deadframe through `deadframe_from_jitframe`, whose `Drop` writes a null over `jf_gcmap`. The force token is the jitframe of a compiled run that is still on the JF shadow stack, inside the residual call that armed `jf_force_descr`; that call site pushed the map before the call and clears it with `pop_gcmap` on return, so the release left the frame's spilled `Ref` slots untraced for the rest of the call. The dynasm backend already separates the two cases, as `FrameData::owning` and `FrameData::borrowing`. Add `owns_gcmap` to `JitFrameDeadFrame` with a `borrowing` constructor, and take it in `force_token_to_dead_frame`. Measured with an instrumented `Drop` reporting the (owns_gcmap, still on the JF shadow stack) pair per drop: across the 44 `getframe` / `vable` / `traceback` synth fixtures, `getframe_caller_locals_nested_compiled_callee` is the one that produces `owns=false on_stack=true`. Restoring the release and forcing a full moving collection at that point changed no fixture's output, so no wrong-answer fixture accompanies this. check.py --backend cranelift 440/440. Assisted-by: Claude
… header The LABEL-header reconcile calls `sync_ref_root_var` for every kept block param whose raw is in `demoted_failarg_slots`. That helper looks the raw up in `ref_root_slots` and emits nothing when it is absent, and a demoted non-ref is never in `ref_root_slots` by construction — so a raw demoted at an earlier LABEL and kept at a later one had its home left alone. `resolve_failarg_opref` tests the demoted offset before it falls back to `use_var`, and a loader re-entry refreshes only the dense carried slot the block param arrives in, so every guard below that LABEL published the value the earlier LABEL's seeding wrote. Store the block param into `demoted_failarg_offset`'s home for that case. Emits nothing on the corpus as it stands: across 110 synth fixtures — the first 50 plus every `bridge` / `retrace` / `nested` / `while` / `loop` / `label` / `preamble` name — the CLIF store count is unchanged, so no fixture currently carries a non-ref demoted at one LABEL and kept at another. check.py --backend cranelift 440/440. Assisted-by: Claude
…er LABEL The LABEL fall-through seeding stored `resolve_opref(raw)` — a `use_var` — into the ref-root home of every demoted ref arg. When the raw was already demoted at an earlier LABEL that is the same frame word, both LABELs deriving the offset from the same `ref_root_slots` entry, and the SSA value stops being refreshed the moment a raw is demoted: `spill_ref_roots` and `reload_ref_roots` both skip a demoted raw, so a collecting call between the two LABELs leaves the register holding a pre-collection pointer while `get_gcmap` marks the word and the collector forwards it in place. The store then wrote that pointer back over the forwarded word. Skip the store when the home is already established. The re-materialize immediately below re-defines the SSA variable from the word. Emits nothing on the corpus as it stands: across the same 110 synth fixtures the CLIF store count is unchanged. check.py --backend cranelift 440/441, with `synth/generator_tree_recursion` discriminated as load — it read 8.6x against a 7.6x gate in the full run at load average 45, and 6.0x re-measured in isolation. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe JIT now supports borrowing deadframes without clearing active GC maps. LABEL handling distinguishes demoted references from non-references, and redundant reference-root synchronization is removed from multiple result-producing operations. ChangesJIT deadframe and root handling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR updates Cranelift GC frame-home handling and removes redundant eager stores; backend tests are green, and the single red performance result is attributed to machine load rather than a functional regression. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…f_gcmap Neither of the two deadframe kinds was covered: the release is invisible to every existing test, and the fix that introduced `borrowing` changes only which of them performs it. Asserts both directions on one off-GC frame — `borrowing` leaves a sentinel `jf_gcmap` alone, `new` nulls it. Verified to fail on the pre-fix behaviour: making the release unconditional again fails the borrowing assertion. Assisted-by: Claude
Two LABELs sharing one descr both demote the carried position, and a `CallMallocNursery` between them collects. The trace reaches the second LABEL by fall-through, so its seeding runs with the SSA value the demoted raw held before the collection. Re-storing that value put a pre-collection address back over the word the collector had forwarded, and the guard below published it. Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2024e5c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
1 participant
Four cranelift changes on the ref-root / demoted-home machinery: one measured
loop-header saving, and three correctness fixes for words the collector reads.
jit(cranelift): drop the ref-root home store at every Ref-defining opTwelve op-emission sites wrote a Ref's ref-root home the moment the op defined
it. No reader needs that store:
spill_ref_rootsover the live slot listimmediately precedes every
emit_push_gcmap, under the same predicateget_gcmapapplies, and a guard's map names dense fail-arg slots only. Ademoted raw's definition always precedes the LABEL that demotes it, and that
LABEL's fall-through seeding writes the same word with the same value.
251 static stores removed. No measurable wall-clock effect — fannkuch
−0.7% over 25 interleaved rounds (12/25 paired), nbody −1.6% (12/20), combined
34/60 at p≈0.30. The stores are off the hot path; the win from the earlier
LABEL-header change was hot-path only.
jit(cranelift): borrow the jitframe's gcmap in force() instead of releasing itforce_token_to_dead_framebuilt its deadframe throughdeadframe_from_jitframe, whoseDropwrites a null overjf_gcmap. The forcetoken is the jitframe of a compiled run that is still on the JF shadow stack,
inside the residual call that armed
jf_force_descr— that call site pushed themap and clears it with
pop_gcmapon return, so the release left the frame'sspilled
Refslots untraced for the rest of the call. The dynasm backendalready separates the two cases as
FrameData::owning/FrameData::borrowing.Adds
owns_gcmaptoJitFrameDeadFramewith aborrowingconstructor.Measured with an instrumented
Dropreporting the (owns_gcmap, still on the JFshadow stack) pair per drop: across the 44
getframe/vable/tracebacksynth fixtures,
getframe_caller_locals_nested_compiled_calleeis the one thatproduces
owns=false on_stack=true. Restoring the release and forcing afull moving collection at that point changed no fixture's output, so the defect
is real but latent and no wrong-answer fixture accompanies it.
jit(cranelift): write a demoted non-ref's frame home at a later LABEL headerThe LABEL-header reconcile called
sync_ref_root_varfor every kept block paramwhose raw is in
demoted_failarg_slots. That helper emits nothing when the rawis absent from
ref_root_slots, and a demoted non-ref never appears there — soa raw demoted at an earlier LABEL and kept at a later one had its home left
alone, while
resolve_failarg_oprefreads that home in preference touse_varand a loader re-entry refreshes only the dense carried slot.
Emits nothing on the corpus as it stands: across 110 synth fixtures the CLIF
store count is unchanged, so no fixture currently carries a non-ref demoted at
one LABEL and kept at another.
jit(cranelift): keep an already-demoted ref's forwarded home at a later LABELThe LABEL fall-through seeding stored
use_var(raw)into the ref-root home. Fora raw demoted at an earlier LABEL that is the same word — both LABELs derive
the offset from the same
ref_root_slotsentry — and its SSA valuestops being refreshed the moment it is demoted —
spill_ref_rootsandreload_ref_rootsboth skip a demoted raw — so a collecting call between thetwo LABELs leaves the register holding a pre-collection pointer that the store
then puts back over the word the collector just forwarded. Skip the store when
the home is already established; the re-materialize immediately below
re-defines the SSA variable from the word.
Two reported defects that did not survive checking
while
get_gcmapmarks it with no liveness test. Harmless: a jitframe iszeroed at allocation (
write_bytes(.., 0, payload_bytes)), and thecustom-trace visitor skips a zero word at
is_nursery_object_startand at thepoison assert. The loader re-seeds its own LABEL's demoted ref and non-ref
homes from the dense carried slots, and an earlier-demoted ref that is a kept
param is already synced.
probe comparing
jf_depth()across everyrun_compiled_codeentry, all 431synth fixtures return at the depth they entered at — including the ~20 that
actually emit CALL_ASSEMBLER (
ca_bridge_multiframe_resume_double_callaloneemits 182).
Gates
pyre/check.py --backend cranelift440/441, jitstats unchanged (nore-record). The one red,
synth/generator_tree_recursion, is discriminated asload: it read 8.6x against a 7.6x gate in the full run at load average 45, and
6.0x re-measured in isolation on the same binary.
cargo test -p majit-backend-cranelift138 + 35 + 1 green,cargo test -p majit-backendgreen.
Test coverage of the three fixes
only_an_owning_deadframe_releases_the_frames_gcmapdrops both deadframe kinds over a frame carrying a sentinel map. Verified to
fail with the condition forced true.
a_later_labels_fallthrough_keeps_an_already_demoted_refs_forwarded_homegives two LABELs one descr so the single back-edge demotes the carried
position at both, collects between them, and reads the guard's published
reference. Verified to fail with the store made unconditional again: it
publishes
GcRef(4358165544), the address the object had before thecollection.
observable path is a loader re-entry, which is the only edge that refreshes
the dense carried slot the block param arrives in without touching the home;
reaching it from a unit test means attaching a bridge, not compiling one
trace. The shape emits nothing on the 110-fixture corpus (CLIF store count
unchanged), so there is no fixture to reach it from either.
Summary by CodeRabbit