jit: give the exact segmentation a total sort key and an explicit owner - #1280
Conversation
`py_exact_by_jit_pc` sorted on the jitcode offset alone and collapsed with `dedup_by_key`. `sort_unstable_by_key` leaves entries with equal keys in an unspecified order, and `dedup_by_key` retains the FIRST of each run, so neither is what the comment above them claimed: that the later-emitted run owns a shared offset. The floor tier's rule is not that one either — its source is a py-indexed first-offset table carrying no run order, so py magnitude is the only tie it can express. Carry the emission index into the sort key and collapse last-wins, which is the stated rule. Assert the offsets are distinct: they are for every one of the 1075 jitcodes the bench corpus compiles, because a run start always addresses a non-`-live-` op and every one of those emits at least one byte. Correct two comments that describe behaviour the code does not have: `filter_liveness_in_place`'s claim that the `-live-` merge collapses run positions onto each other, and the tie-break claim above. Give `abort_permanent_py_pc_by_jit_pc` the same total sort key. 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 (1)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review. WalkthroughThe codewriter now documents remapping order, sorts abort-PC markers deterministically, and preserves deterministic ownership when exact PC runs share offsets. ChangesJIT PC mapping
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes segmentation ownership deterministic and adds invariant checks without changing the published table; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ec9732e). 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
|
Follows up the CodeRabbit review on #1268, which flagged the exact-segmentation
dedup and was merged unaddressed.
What the review got right, and what it got wrong
Three code-level facts held:
py_exact_by_jit_pc.sort_unstable_by_key(|&(off, _)| off)keys on the offsetalone, so equal offsets are left in an unspecified order.
dedup_by_keyretains the first entry of each run.floor tier's later-py-wins tie break".
So the code implemented neither its own stated rule nor the floor tier's. The
floor tier cannot express "later-emitted" at all: its source is a py-indexed
first-offset table with no run order in it, so py magnitude is its only tie.
Those are two different rules, and the comment conflated them.
Two parts of the review's claim did not hold, and are worth recording so the
finding is not re-opened at the wrong severity:
addresses a non-
-live-Insn::Op; every one of those reachesstart_instrand emits at least one byte (the
dispatch_opfallthrough is apanic!).remove_repeated_live's remap is many-to-one only over the-live-positionsit folds, and the three splice passes only insert and shift monotonically.
Measured to agree:
PYRE_VSTACK_EXACT_AUDITover the bench corpus reportsruns_recorded == translatedfor 1075/1075 jitcodes across 442 programs —the dedup never dropped an entry.
sort_unstable_by_keyhas no randomness source, and the input is already non-decreasing in the
offset, so the sort is a no-op on the production input. Had a duplicate
existed, the winner would have been deterministically wrong, not flaky.
The consumer is sensitive —
exact_py_pc_for_jitcode_pcfeedsvstack_step_py_pc, which is the walk mirror's coordinate — so the wholedefence rests on the duplicate never existing, and nothing asserted that.
The change
implements the rule its comment states, under any input.
debug_assert!that the offsets are distinct, turning a future pass thatdeletes an ordinary op (or an encoder arm that emits nothing) into a loud
failure here rather than a silent wrong coordinate in the mirror.
filter_liveness_in_place's claim that the-live-merge collapses runpositions onto each other, and the tie-break claim.
abort_permanent_py_pc_by_jit_pcthe same total sort key. Its markerseach emit an opcode byte so its dedup is a no-op too, but the partial key had
the same shape.
Verification
collisions — the explicit collapse never fires, so the published table is
unchanged.
debug_assertexercised: debug build over 40 synth benches, 0 firings.cargo test --all --no-default-features --features dynasm --no-fail-fast:rc=0, 138 test binaries.
pyre/check.py: dynasm 436/436, cranelift 436/436. The one wasm red isshort_circuit_value_kept_stackat ratio 4.6x > 4x, which is pre-existing onthis host and unrelated to this change.
— authored by Claude
Summary by CodeRabbit