Repository navigation
jit: exempt LOAD_FAST_CHECK's null arm from the loop-body abort_permanent scan, and give markers an exact jitcode -> Python-PC inverse - #1023
Merged
Conversation
`loop_body_abort_permanent_pc` scans to `code.len()` instead of the loop's back edge when the loop header is covered by an exception handler. No synthetic fixture responded to that widening: sweeping all 356 with the scan narrowed left `loops_compiled`, `bridges_compiled`, `loops_aborted` and `guard_failures` unchanged on every one. Three fixtures for the shape. Each has a hot loop inside a `try` and a post-loop tail that reads the loop variable, which the compiler spells LOAD_FAST_CHECK because the raise can reach the handler before the loop assigns it. LOAD_FAST_CHECK splits on a conditionally-bound slot and its null arm emits the `abort_permanent` the widened scan finds; the marker sits in a dead-end block reached only when the local really is unbound, and `unbound_tail` is the arm that reaches it. Delivery flavours covered: builtin container, int-specialized arithmetic guard, non-matching inner handler, inlined callee, a raise in the post-loop tail, a second loop after the handler, and two loops under one `try`. `PYRE_FBW_LOOPBODY_SCAN_LOOP_ONLY` drops the widening without a rebuild, the counterpart of the existing `PYRE_FBW_LOOPBODY_SCAN_FULL`. With it set, all three fixtures fail `jit-stats regression: guard_failures 20 -> 40/60/80`. Output is byte-identical across pypy3, python3.14 and all three backends, and the recorded baselines agree field-for-field across dynasm, cranelift and wasm. Assisted-by: Claude
`loop_body_abort_permanent_pc`'s doc comment names "the `DELETE_FAST` cleanup for `except X as e`" as the post-loop marker the widened scan catches. Across the shapes `bench/synth/loop_in_try_*` cover, every marker it catches is `LOAD_FAST_CHECK`'s null arm instead. A loop inside a `try` is what makes the tail read a loop variable through LOAD_FAST_CHECK rather than LOAD_FAST — the raise can reach the handler before the loop assigns the slot, so the slot is only conditionally bound on the join. The codewriter splits that opcode, compiles the bound arm, and sends the null arm to a dead-end block whose `abort_permanent` no bound-local run reaches. Also records why the marker cannot simply be exempted: the null arm's block is emitted after the whole body, and `py_floor_by_jit_pc` keys each Python PC to its first jitcode offset, so the floor lookup attributes the late block to `RETURN_VALUE` rather than to the LOAD_FAST_CHECK that owns it. Comment only; no behaviour change. Assisted-by: Claude
`PyErrorKind::GeneratorReturn` had six references in the tree: the variant, its `ExcKind::RuntimeError` mapping, three handlers in `eval_loop` / `handle_exception`, and a comment. No constructor. Nothing ever raised it, so the handlers were unreachable. `Instruction::ReturnGenerator` does not unwind: `return_generator` (eval.rs:4375) is a bare `push(w_none())` for the following POP_TOP, because the generator object is already built at call time in `call_user_function_with_eval`. Verified with cargo test --all --no-default-features --features dynasm (7396 tests), check.py --synthetic-only --backend dynasm (359/359), and the cpython_tests dynasm legs for test_generators, test_yield_from, test_coroutines, test_generator_stop and test_exceptions (no regressions). Assisted-by: Claude
…nent scan `loop_body_abort_permanent_pc` declined a whole frame on any `abort_permanent` in its scan window. `LOAD_FAST_CHECK` lowers to a `ptr_nonzero` split whose null arm ends in a dead-end block carrying that marker, so the scan refused frames over a conditionally-bound local read. Skip markers whose owning opcode is `LoadFastCheck`. Naming the owning opcode needs an exact jitcode-offset -> Python-PC inverse: `py_floor_by_jit_pc` keys each Python PC to its FIRST jitcode offset, and the null arm's block is emitted after the whole body, so the floor lookup resolves it to whichever PC last opened a segment. Record every `abort_permanent`'s `insns` index in `SSARepr` at `serialize_op`, carry it through the three `-live-` splicing passes and the `remove_repeated_live` remap, translate it in the `finish_with_positions_from` batch alongside `pc_map`, and publish it as `PyJitCodeMetadata::abort_permanent_py_pc_by_jit_pc`. `SSARepr::stream_positions_mut` replaces the per-table position rewrite loops in the splicing passes. Re-recorded the three `loop_in_try_*` baselines on all three backends: `loops_compiled` 1 -> 3/4/5, `guard_failures` 20 -> 60/80/100, identical across dynasm/cranelift/wasm. Output stays byte-exact against pypy3 and python3.14. Min-of-9 user+sys CPU, dynasm: 0.65s -> 0.19s, 1.33s -> 0.20s, 0.97s -> 0.18s. Assisted-by: Claude
Drops the separate `wasm-build` matrix job. The Linux job that runs pyre/check.py now builds `-p pyre-wasm --features web` before check.py's own wasm-host build, so both bindings share the release target directory instead of paying a second checkout, cache restore and LLBC download. Assisted-by: Claude
…W_DEBUG_ABORT The `[fbw-abort] start_pc=… abort_permanent_pc=…` line reported a bare jitcode offset. Resolve it through `abort_permanent_py_pc_by_jit_pc` and append `py_pc=<n> <Instruction>`. `marker_is_load_fast_check_null_arm`'s table lookup and CodeObject decode move into `abort_permanent_owner`, which both callers share; the predicate becomes a `matches!` on its result. The line only prints under `PYRE_FBW_DEBUG_ABORT`, which check.py never sets, and no fixture in the synthetic suite currently reaches this decline. 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 (20)
💤 Files with no reviewable changes (1)
WalkthroughThe PR standardizes generator-return exception dispatch, preserves abort-permanent JIT positions through code generation, resolves marker owners during tracing, adds exception-loop benchmarks, and moves the WebAssembly web-binding build into Linux CI. ChangesJIT exception handling
Linux WebAssembly CI
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PythonCode
participant PyJitCodeWriter
participant PyJitCodeMetadata
participant TraceScanner
PythonCode->>PyJitCodeWriter: compile loop with abort-permanent markers
PyJitCodeWriter->>PyJitCodeMetadata: store JIT offsets and Python PCs
PyJitCodeMetadata->>TraceScanner: provide marker ownership metadata
TraceScanner->>TraceScanner: scan loop body or widened try tail
TraceScanner-->>PythonCode: report resolved abort diagnostics
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 |
![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
https://github.com/youknowone/pyre/blob/072fd2cc303729ae6391f56e4c4f8d8985bac476/pyre-jit-trace/src/trace.rs#L4853-L4856
When a hot loop reaches a post-loop LOAD_FAST_CHECK whose conditionally bound local is actually null, this exemption allows the walk to reach the marker reactively. However, the resulting AbortPermanentMarkerReached is still translated by fbw_abort_resume_py_pc through containing_py_pc_for_jitcode_pc, even though the new metadata explicitly documents that this floor lookup attributes the late null-arm block to an unrelated opcode. The abort-state flush can therefore resume at the wrong Python instruction, skipping or re-executing bytecode instead of raising UnboundLocalError; the reactive abort path needs to consume abort_permanent_py_pc_by_jit_pc too. This violates the repository invariant that generated JIT behavior preserve interpreter semantics.
AGENTS.md reference: AGENTS.md:L16-L19
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 072fd2c). 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
|
youknowone
added a commit
that referenced
this pull request
Aug 4, 2026
…nent scan, and give markers an exact jitcode -> Python-PC inverse- #1023 (#1032) * bench: cover the loop_in_try abort_permanent scan widening `loop_body_abort_permanent_pc` scans to `code.len()` instead of the loop's back edge when the loop header is covered by an exception handler. No synthetic fixture responded to that widening: sweeping all 356 with the scan narrowed left `loops_compiled`, `bridges_compiled`, `loops_aborted` and `guard_failures` unchanged on every one. Three fixtures for the shape. Each has a hot loop inside a `try` and a post-loop tail that reads the loop variable, which the compiler spells LOAD_FAST_CHECK because the raise can reach the handler before the loop assigns it. LOAD_FAST_CHECK splits on a conditionally-bound slot and its null arm emits the `abort_permanent` the widened scan finds; the marker sits in a dead-end block reached only when the local really is unbound, and `unbound_tail` is the arm that reaches it. Delivery flavours covered: builtin container, int-specialized arithmetic guard, non-matching inner handler, inlined callee, a raise in the post-loop tail, a second loop after the handler, and two loops under one `try`. `PYRE_FBW_LOOPBODY_SCAN_LOOP_ONLY` drops the widening without a rebuild, the counterpart of the existing `PYRE_FBW_LOOPBODY_SCAN_FULL`. With it set, all three fixtures fail `jit-stats regression: guard_failures 20 -> 40/60/80`. Output is byte-identical across pypy3, python3.14 and all three backends, and the recorded baselines agree field-for-field across dynasm, cranelift and wasm. Assisted-by: Claude * jit: record what the loop_in_try scan widening actually declines `loop_body_abort_permanent_pc`'s doc comment names "the `DELETE_FAST` cleanup for `except X as e`" as the post-loop marker the widened scan catches. Across the shapes `bench/synth/loop_in_try_*` cover, every marker it catches is `LOAD_FAST_CHECK`'s null arm instead. A loop inside a `try` is what makes the tail read a loop variable through LOAD_FAST_CHECK rather than LOAD_FAST — the raise can reach the handler before the loop assigns the slot, so the slot is only conditionally bound on the join. The codewriter splits that opcode, compiles the bound arm, and sends the null arm to a dead-end block whose `abort_permanent` no bound-local run reaches. Also records why the marker cannot simply be exempted: the null arm's block is emitted after the whole body, and `py_floor_by_jit_pc` keys each Python PC to its first jitcode offset, so the floor lookup attributes the late block to `RETURN_VALUE` rather than to the LOAD_FAST_CHECK that owns it. Comment only; no behaviour change. Assisted-by: Claude * interp: remove the never-constructed GeneratorReturn error kind `PyErrorKind::GeneratorReturn` had six references in the tree: the variant, its `ExcKind::RuntimeError` mapping, three handlers in `eval_loop` / `handle_exception`, and a comment. No constructor. Nothing ever raised it, so the handlers were unreachable. `Instruction::ReturnGenerator` does not unwind: `return_generator` (eval.rs:4375) is a bare `push(w_none())` for the following POP_TOP, because the generator object is already built at call time in `call_user_function_with_eval`. Verified with cargo test --all --no-default-features --features dynasm (7396 tests), check.py --synthetic-only --backend dynasm (359/359), and the cpython_tests dynasm legs for test_generators, test_yield_from, test_coroutines, test_generator_stop and test_exceptions (no regressions). Assisted-by: Claude * jit: exempt LOAD_FAST_CHECK's null arm from the loop-body abort_permanent scan `loop_body_abort_permanent_pc` declined a whole frame on any `abort_permanent` in its scan window. `LOAD_FAST_CHECK` lowers to a `ptr_nonzero` split whose null arm ends in a dead-end block carrying that marker, so the scan refused frames over a conditionally-bound local read. Skip markers whose owning opcode is `LoadFastCheck`. Naming the owning opcode needs an exact jitcode-offset -> Python-PC inverse: `py_floor_by_jit_pc` keys each Python PC to its FIRST jitcode offset, and the null arm's block is emitted after the whole body, so the floor lookup resolves it to whichever PC last opened a segment. Record every `abort_permanent`'s `insns` index in `SSARepr` at `serialize_op`, carry it through the three `-live-` splicing passes and the `remove_repeated_live` remap, translate it in the `finish_with_positions_from` batch alongside `pc_map`, and publish it as `PyJitCodeMetadata::abort_permanent_py_pc_by_jit_pc`. `SSARepr::stream_positions_mut` replaces the per-table position rewrite loops in the splicing passes. Re-recorded the three `loop_in_try_*` baselines on all three backends: `loops_compiled` 1 -> 3/4/5, `guard_failures` 20 -> 60/80/100, identical across dynasm/cranelift/wasm. Output stays byte-exact against pypy3 and python3.14. Min-of-9 user+sys CPU, dynasm: 0.65s -> 0.19s, 1.33s -> 0.20s, 0.97s -> 0.18s. Assisted-by: Claude * ci: build the pyre-wasm web binding inside the Linux check.py job Drops the separate `wasm-build` matrix job. The Linux job that runs pyre/check.py now builds `-p pyre-wasm --features web` before check.py's own wasm-host build, so both bindings share the release target directory instead of paying a second checkout, cache restore and LLBC download. Assisted-by: Claude * jit: name the opcode that owns a declining abort_permanent in PYRE_FBW_DEBUG_ABORT The `[fbw-abort] start_pc=… abort_permanent_pc=…` line reported a bare jitcode offset. Resolve it through `abort_permanent_py_pc_by_jit_pc` and append `py_pc=<n> <Instruction>`. `marker_is_load_fast_check_null_arm`'s table lookup and CodeObject decode move into `abort_permanent_owner`, which both callers share; the predicate becomes a `matches!` on its result. The line only prints under `PYRE_FBW_DEBUG_ABORT`, which check.py never sets, and no fixture in the synthetic suite currently reaches this decline. Assisted-by: Claude
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six commits. The centerpiece is a JIT decline that refused whole frames over a
dead-end block no bound-local run ever reaches; the rest are the fixtures that
pin it, a doc correction, a dead error kind, a CI job merge, and a diagnostic.
jit: exempt LOAD_FAST_CHECK's null arm from the loop-body abort_permanent scanloop_body_abort_permanent_pcdeclined a whole frame on anyabort_permanentin its scan window. That refusal exists for unportedopcodes, whose arm bails without modelling stack effect — a mis-seeded loop
guard exits on pass 1 and double-runs the tail.
LOAD_FAST_CHECKis ported. It lowers to aptr_nonzerosplit: the codewritercompiles the bound arm normally and sends the null arm to a dead-end block
ending in
emit_abort_permanent!. Neither walk outcome mis-seeds anything, sothe marker is exempt — and exempt everywhere in the window, not just in the
tail. (
loop_end, the max back-edge jitcode pc, is not a source-position proxy:loop_in_try_tail_raise_and_second_loop's source-tail marker measures2067 < loop_end 3363.)Naming the owning opcode is the whole difficulty.
py_floor_by_jit_pcis builtfrom each Python PC's first jitcode offset, and the null arm's block is
emitted after the whole body, so the floor lookup resolves it to whichever PC
last opened a segment —
RETURN_VALUE, not theLOAD_FAST_CHECKthat owns it.So markers get an exact inverse.
SSARepr::abort_permanent_insn_posrecordsevery marker's
insnsindex atserialize_op; it is carried through the three-live-splicing passes and theremove_repeated_liveremap, translated in thefinish_with_positions_frombatch alongsidepc_map, and published asPyJitCodeMetadata::abort_permanent_py_pc_by_jit_pc. Only markers get this —there are a handful per jitcode, and they are the one op class a whole-frame
decision hangs on.
SSARepr::stream_positions_mutreplaces the per-table position-rewrite loops inthe splicing passes, so a newly added side table cannot silently miss a pass.
That hand-off is five sites, and missing any one leaves the table empty at
runtime with no error.
Result — the three new fixtures, on all three backends:
loops_compiledguard_failuresloop_in_try_raise_into_handlerloop_in_try_tail_raise_and_second_looploop_in_try_tail_unbound_checkloops_aborted=0,bridges_compiled=0throughout. Every field is identicalacross dynasm, cranelift and wasm, and output is byte-exact against pypy3 and
python3.14.
bench: cover the loop_in_try abort_permanent scan wideningThe widening (
scan_end = code.len()when the loop header is covered by anexception handler) had no fixture responding to it: sweeping all 356
synthetic fixtures with the scan narrowed left
loops_compiled,bridges_compiled,loops_abortedandguard_failuresunchanged on every one.Three fixtures for the shape. Each has a hot loop inside a
tryand a post-looptail reading the loop variable — which the compiler spells
LOAD_FAST_CHECKprecisely because the raise can reach the handler before the loop assigns the
slot. Delivery flavours covered: builtin container, int-specialized arithmetic
guard, non-matching inner handler, inlined callee, a raise in the post-loop
tail, a second loop after the handler, and two loops under one
try.PYRE_FBW_LOOPBODY_SCAN_LOOP_ONLYdrops the widening without a rebuild (thecounterpart of the existing
PYRE_FBW_LOOPBODY_SCAN_FULL). With it set, allthree fixtures fail
guard_failures 20 -> 40/60/80— the fixtures gate thewidening in both directions.
jit: record what the loop_in_try scan widening actually declinesThe doc comment named "the
DELETE_FASTcleanup forexcept X as e" as thepost-loop marker the widened scan catches. Across every shape these fixtures
cover, it is
LOAD_FAST_CHECK's null arm instead. Comment only; landed beforethe fix so the wrong attribution is not what the fix appears to rest on.
interp: remove the never-constructed GeneratorReturn error kindPyErrorKind::GeneratorReturnhad six references — the variant, itsExcKind::RuntimeErrormapping, three handlers ineval_loop/handle_exception, and a comment — and no constructor. The handlers wereunreachable.
Instruction::ReturnGeneratordoes not unwind:return_generatoris a bare
push(w_none())for the followingPOP_TOP, because the generatorobject is already built at call time in
call_user_function_with_eval.ci: build the pyre-wasm web binding inside the Linux check.py jobDrops the separate
wasm-buildmatrix job; the Linux check.py job builds-p pyre-wasm --features webbefore check.py's own wasm-host build, so bothbindings share the release target directory instead of paying a second checkout,
cache restore and LLBC download.
jit: name the opcode that owns a declining abort_permanent in PYRE_FBW_DEBUG_ABORT[fbw-abort]reported a bare jitcode offset; it now resolves through the newinverse and appends
py_pc=<n> <Instruction>. One line instead of aMAJIT_DUMP_SSAREPR+last_instr = py_pc - 1hunt.decline, so the line is only observable on a routing-patched binary or under
PYRE_FBW_LOOPBODY_SCAN_FULL=1. It is gated behind an env var check.py neversets.
Verification
cargo test --all --no-default-features --features dynasm— pass (101 testbinaries).
cargo fmt --check— clean.check.pyat the measurement base (origin/main@40c82e5c6a9) had 8 dynasm /16 cranelift / 14 wasm failures. All 38 are inherited, not introduced.
Proven, not asserted: a control binary was built with only this branch's five
source files reverted to
origin/main, on both dynasm and cranelift, with LLBCre-extracted per leg and the fingerprint checked (
pyre-jitre-extracted eachswap; the other crates correctly skipped). Every affected fixture's jitstats
came back 26/26 byte-identical to the control, while the positive control —
this branch's three fixtures — correctly differed. #1019 subsequently
re-recorded exactly the eight dynasm baselines flagged here, confirming the
attribution independently.
The branch is two commits behind
origin/mainas of opening (#1018, #945, bothof which re-recorded a large set of baselines).
git merge-treereports a cleanmerge; re-verification against the newer base has not been run, so the failure
counts above are stated against the base they were measured on.
Summary by CodeRabbit
Bug Fixes
Performance
Testing