wasm: pick loop-header label by JUMP descr; jit: scope abort_permanent to loop body#558
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
![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: f4f1d5bc0d
ℹ️ 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".
| if ops.iter().rposition(|op| op.opcode == OpCode::Label) != Some(loop_label) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When a peeled trace has real work before the JUMP target but the terminal JUMP targets a non-last LABEL (the shape this change says now compiles as a plain local loop), returning false here also makes compile_loop record has_preamble = false. That path publishes the first label as a normal key-0 entry, so a loop-closing bridge to that label can be accepted and tail-call the function entry instead of a label loader; the peeled preamble then runs again with loop-carried values, which can livelock or corrupt induction state. If key dispatch is unsupported for this shape, the loop still needs to be marked as having a preamble or its labels must not be published as non-peeled entries.
Useful? React with 👍 / 👎.
| let Some(merge_point) = crate::jitcode_runtime::decoded_ops(code) | ||
| .find(|op| op.opname == "jit_merge_point") | ||
| .map(|op| op.pc) |
There was a problem hiding this comment.
For a function with multiple hot loops, this always selects the first jit_merge_point in the whole jitcode rather than the one corresponding to the start_pc being traced. If the later hot loop contains an in-body abort_permanent and the first loop is clean, back_edge_end is computed from the first loop's back edge, the later marker falls after scan_end, and the upfront decline returns false; with PYRE_FULL_BODY_WALK enabled the authoritative walk can then hit the unsupported op and reproduce the frame/tail corruption this guard is meant to prevent. The scan should anchor on the merge point for the current start_pc.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 378bf97). 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
|
full_body_walk_trace declines a loop trace up front when loop_body_has_abort_permanent finds an abort_permanent opcode. The scan ran from the first jit_merge_point to the end of the per-CodeObject jitcode, so a marker in post-loop code declined the hot loop even though the walk never reaches it. `except X as e` lowers its implicit cleanup to DELETE_FAST (an abort_permanent) in the post-loop handler, so any hot loop in a function with an `except ... as e` failed to compile and ran interpreted. Scope the scan to the loop body: from the first jit_merge_point up to the loop's back-edge (the last goto-family op whose decoded label targets at or before the header). A loop whose header is covered by an exception handler (lookup_exceptiontable) keeps the full-tail scan, so a post-loop abort_permanent still declines it — compiled-loop delivery of an uncaught raise into the handler is not yet supported. In-loop markers (the SWAP a chained comparison lowers to) stay before the back-edge and still decline. PYRE_FBW_LOOPBODY_SCAN_FULL opts back into the full-tail scan. Assisted-by: Claude
find_label_args and the loop-header index used the last LABEL of a multi-label trace. A terminal JUMP whose descr targets an earlier LABEL then bound the parallel move and the emitted `loop` to the wrong label, cross-loading loop-carried locals. find_jump_target_label_index / find_loop_label_index resolve the target LABEL by descr Arc identity and fall back to the last LABEL for descr-less IR. is_resumable_peeled declines the key-dispatch shape unless the JUMP target is the last LABEL. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9242dddc
ℹ️ 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 loop_in_try = unsafe { | ||
| pyre_interpreter::pycode::w_code_lookup_exceptiontable( | ||
| w_code as pyre_object::PyObjectRef, | ||
| (start_pc as u32) * 2, | ||
| ) |
There was a problem hiding this comment.
When a hot loop contains an inner try/except ... as e but the loop header itself is outside the protected range, this lookup returns None because it probes only start_pc. Handler cleanup can be laid out after the normal loop backedge, and DELETE_FAST cleanup is emitted as abort_permanent (pyre/pyre-jit/src/jit/codewriter.rs:11292-11308); with loop_in_try == false, scan_end cuts at the normal backedge and misses that marker. In that exception path the full-body walk can execute handler side effects and then abort/replay instead of declining up front.
Useful? React with 👍 / 👎.
1 participant
Two related JIT correctness changes on the
ec-wiringbranch.jit: scope loop abort_permanent decline to the loop body
loop_body_has_abort_permanentwas over-broad: anexcept X as eanywhere in afunction marked the whole hot loop abort-permanent, so the loop never
JIT-compiled and the exc/resume path stayed in the interpreter. Scoping the
decline to the loop body lets these loops compile.
wasm: select loop-header label by terminal JUMP's descr
The wasm backend chose the loop-header label of a multi-label (peeled) trace by
taking the last
LABEL, not the label the terminalJUMPactually targets.When the JUMP's descr points at a non-last label,
find_label_args(theback-edge parallel move) and
loop_label_idx(the emittedloop) bound to thewrong label, so loop-carried locals cross-loaded — a loop-invariant
range-iterator local read back as an unrelated int and
FOR_ITERraisedTypeError: not an iterator.find_jump_target_label_index/find_loop_label_indexnow resolve the targetlabel by the JUMP descr's identity (falling back to the last label for
descr-less IR), and
is_resumable_peeleddeclines the key-dispatch shape unlessthe JUMP target is the last label (such shapes compile as a plain local loop).
This is confined to
majit-backend-wasm/src/codegen.rs; the native dynasm andcranelift backends do not link it and are unaffected.
Verification
is_resumable_peeledchange)for _ in range(N): s += Gloop whosepeeled trace's terminal JUMP targets a non-last label; the corrupted
loop-carried iterator (read back as an int) is corrected by the label-selection
fix.
🤖 Generated with Claude Code