jit: coerce a closing JUMP to the label arity only when it targets that label - #1127
Conversation
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f30c25e8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let jump_targets_local_label = matches!( | ||
| (jump_target_descr_idx, local_label_descr_idx), | ||
| (Some(jump_idx), Some(label_idx)) if jump_idx == label_idx |
There was a problem hiding this comment.
When external closes are admitted—the stated use case for this change—distinct TargetTokens can share the same numeric token_id because it is only local to a JitCellToken; history.rs explicitly says backend identity is the descriptor's Arc address. Comparing Descr::index() therefore misclassifies an external target with the same local number as this label, causing the new branch to truncate or pad its arguments and recreating the cross-token arity corruption. Compare the descriptor Arcs with Arc::ptr_eq/descr_identity instead, and make the regression test use distinct descriptors with the same index.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b988673). 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)
4. Structural adaptations
|
…at label `assemble_peeled_trace_with_jump_args` applied compile.py:334's equal-arity rule to every close whose `jump_to_self` flag was set. That flag records that the closing JUMP was redirected onto some existing target token, not that it targets the local label, so a cross-token JUMP was truncated to the local label's arity. Rename the flag to `jump_was_redirected` and compare the JUMP's target descr against the local label's before coercing; a JUMP carrying another token's virtual state arity (unroll.py:346-357) now passes through unchanged. Over all 417 corpus scripts every one of the 543 redirected closes targets its local label, so no bench moves: `jump_to_existing_trace` still discards an external close before assembly. Correct that discard site's comment, which attributed the fallback to the cranelift one-function-per-trace layout — admitting the close on dynasm breaks as well. Add `synth/retrace_outer_loop_type_flip`, a nested loop whose accumulator flips int -> float in the outer loop, recorded at `loops_aborted=2` and `retraces_compiled=0`. Assisted-by: Claude
|
Rebased onto Where the verification stands. The full dynasm bench corpus (411 benches, zero Follow-up, not in this PR. The pyre instead seeds That predicts the split seen when the discard was lifted: the five benches that — commented by Claude |
|
CI triage — the Both failures are in the
main's own run at this PR's exact base — commented by Claude |
1 participant
assemble_peeled_trace_with_jump_argsappliedcompile.py:334's equal-arity ruleto every close whose
jump_to_selfflag was set. That flag records that theclosing JUMP was redirected onto some existing target token, not that it
targets the local label, so a cross-token JUMP was truncated to the local label's
arity — 8 args to 7, dropping a trailing
ConstInt(1)that pypy spends on aliteral in the same slot.
This renames the flag to
jump_was_redirectedand compares the JUMP's targetdescr against the local label's before coercing. A JUMP carrying another token's
virtual-state arity (
unroll.py:346-357) now passes through unchanged;compile.py:334asserts equality for the same-label close only.Also corrects the comment at the
jump_to_existing_tracediscard site, whichattributed the fallback to cranelift's one-function-per-trace layout. That was
measured false: admitting the external close breaks dynasm too, which relocates
every LABEL and refuses a JUMP below the first page, so the branch does name a
live address.
Reachability
The change is a no-op on today's corpus, measured rather than argued. Every
bench script (394 synth + 23 top-level = 417) was swept with the release binary
under
MAJIT_LOG=1, tallying both sides of the new predicate:redirected/targets_localEvery redirected close targets its local label, because the discard at
jump_to_existing_traceremoves an external close before assembly ever sees it.The fix becomes load-bearing when that discard is lifted.
Fixture
pyre/bench/synth/retrace_outer_loop_type_flip.py— a nestedwhilewhoseaccumulator flips
int -> floatin the outer loop, so the retrace grown forit closes onto the token the inner loop left behind. Recorded at
loops_compiled=1 loops_aborted=2 retraces_compiled=0 guard_failures=590,identical on all three backends. That is pyre's current declined state, and it
is the acceptance test for whatever admits the close.
Verification
cargo test -p majit-metainterp --no-default-features --features dynasm— 1457 passedcargo fmtcleanpyre/check.py --backend dynasmat base1a63e36e37a— full bench corpus (411) with zero failuresb925c3e5ead: dynasm 410/411, cranelift 410/411, wasm 403/407, where all 6 failures were the check.py: derive the pypy floor from the ceiling, and refit the ceilings to measured ratios #1086 baseline reds since restored by parity: say why a runner failed, then fix the five Windows failures it was hiding #1104New regression test
test_assemble_peeled_trace_keeps_external_target_jump_aritypins the pass-through; the existing
test_assemble_peeled_trace_passes_through_resolved_body_inputswas corrected tomodel the same-label close it was actually exercising.
— authored by Claude