jit: residualise the whole int-box tail, and decline a kept-stack branch holding a NULL ConstPtr - #1131
Merged
Conversation
Fold `w_int_gc_alloc` into a new `w_int_box_slow` carrying both the collector arm and the `malloc_typed` fall-through, and move the `dont_look_inside` boundary onto it. `w_int_new` keeps its tagged and prebuilt fast paths in the trace and calls the tail. A stack-built `W_IntObject` lowers to a `SyntheticTransparentCtor` for its `PyObject` header, whose funcptr constant degrades to a `symbolic_fnaddr` hash. A descending sub-jitcode walk cannot record such a call, so it declines the whole descent; that is what took `list.pop()`'s fold off the compiled loop. With the boundary around the pair, `bench/synth/list_pop_append` runs 0.35s at 6.7x on dynasm and 0.43s at 10.1x on cranelift. `jit_fnaddr` binds the renamed trampoline under both alias spellings. Assisted-by: Claude
…LL ConstPtr Extracted from #1118. Add `kept_stack_has_null_const_slot` and take it as Hazard (4) in `guarded_branch_core`, with a matching `decline-why` field. The NULL `ConstPtr` encoding is what a genuine null operand and an unset vable shadow slot both decode to, so a kept operand-stack slot holding one has no source the resume snapshot can name; when the not-taken edge decodes no `ref_copy` moves there is no fallback either, and the resume rebuilds the slot NULL. The decline is narrower than "the mirror does not cover" — a mirror shorter than the resume depth and a `NONE` hole both still resume through the shadow. Also record why the FOR_ITER route in `try_walker_specialize_seqiter_getitem_next` passes `entry_is_call_boundary`: FOR_ITER peeks its single operand where the operator opcodes pop theirs. `test.test_re` goes FAIL -> PASS on dynasm. Re-record `bench/synth/list_append_write_barrier_gc` on all three backends: bridges_compiled 5 -> 3, guard_failures 1345 -> 938, loops_aborted 1 -> 2, loops_compiled 12 -> 11. Assisted-by: Claude
Comment only. `wrapint` (`objspace/std/intobject.py`) keeps its allocation inline — its own comment there says the function is inlined into every caller — so the residualisation boundary is a deviation. Note that the orthodox lowering is `new_with_vtable`, that `fuse_boxing_alloc` is the pass meant to produce it, and the measurement that it produces it nowhere: 134 candidate sites, all reporting the vtable unresolved, because `resolve_vtable_addr` reads a `HostStaticAddrs.pytypes` that is empty in the build-script pipeline the pass runs in. 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 (8)
WalkthroughThe JIT now detects null constant slots in kept operand-stack mirrors and permanently aborts unsafe branches. Integer allocation uses ChangesJIT GC safety and allocation
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant JITBranchDispatch
participant kept_stack_has_null_const_slot
participant KeptStackDiagnostics
JITBranchDispatch->>kept_stack_has_null_const_slot: Inspect target resume depth and vstack boxes
kept_stack_has_null_const_slot-->>JITBranchDispatch: Return null constant slot hazard
JITBranchDispatch->>KeptStackDiagnostics: Report hazard and recovered-move status
Possibly related issues
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 |
![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/ad60244ca57b4a94ab1a355c11f28759abd7b62e/pyre-object/src/intobject.rs#L140
With the current defaults CAN_BE_TAGGED=false and WITHPREBUILTINT=false, every w_int_new call reaches this dont_look_inside boundary, turning every exact-int allocation into a residual call. Unlike upstream wrapint, this prevents the optimizer from seeing NewWithVtable and eliminating non-escaping boxes in arithmetic-heavy loops. The unresolved vtable table is a translator defect to repair before landing this change, rather than a reason to replace the inline allocation with a permanent residual call.
AGENTS.md reference: AGENTS.md:L231-L233
https://github.com/youknowone/pyre/blob/ad60244ca57b4a94ab1a355c11f28759abd7b62e/pyre-jit-trace/src/jitcode_dispatch/mod.rs#L9313-L9317
For a multi-slot kept stack, any unrelated recovered ref_copy makes resolved_recovered nonempty and suppresses this hazard even when the NULL slot itself has no matching recovery. collect_outer_active_boxes keys recovery by destination color and deliberately does not report a NULL mirror slot as unsourced, so that slot then falls through to the stale or unwritten resume register and can reproduce the missing-argument miscompile; conversely, an empty recovery permanently declines an otherwise valid guard and already reduces compiled loops and bridges in the updated baseline. Track recovery per slot—or, preferably, distinguish unset shadow entries from genuine PUSH_NULL values—instead of using this global workaround.
AGENTS.md reference: AGENTS.md:L252-L254
ℹ️ 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 ad60244). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
youknowone
added a commit
that referenced
this pull request
Aug 10, 2026
`guard_failures` 13 -> 2 with `loops_compiled` unchanged at 2. Not this branch's doing: reverting this branch's three metainterp files and rebuilding the wasm module reads the same 2, so the move belongs to the base this branch was rebased onto — `jit: residualise the whole int-box tail, and decline a kept-stack branch holding a NULL ConstPtr` (#1131). `check.py` reports the drop as IMPROVED and refuses to pass until the baseline is recorded. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 10, 2026
… path with unroll.py, and fix the wasm livelock it exposed (#1134) * wasm: decide a terminal JUMP's locality by its target token, not by whether the trace carries a LABEL `has_cross_loop_terminal_jump` answered `has_jump && !has_label`, while the code generator answers the same question by descr identity in `find_loop_label_index` — `x86/assembler.py:2463`'s `target_token in self.target_tokens_currently_compiling`. The two disagree on a trace that defines LABELs of its own and whose terminal JUMP names none of them: codegen takes the external arm and emits `return_call_indirect(external_jump_slot)`, but the predicate answered false, so neither `compile_loop` nor `compile_bridge` called `resolve_cross_loop_jump_target` and the arm ran with `external_jump_key = 0` and `external_jump_slot = source_func_handle`. Key 0 re-enters the target at its function entry, so a peeled target re-runs its preamble against mid-loop state and the induction variable never advances — the livelock `compile_bridge` already documents above `bridge_is_loop_closing`, reached through a shape the predicate did not recognise. A tail call keeps the stack and heap flat, so it presents as a hang, not a crash. Both callers keep the one predicate. Widening is safe at each: they either resolve a real target through `resolve_cross_loop_jump_target` — adopting its frozen frame geometry, which a tail call requires anyway — or decline with `BackendError::Unsupported`, dropping the trace back to the interpreter. Assisted-by: Claude * jit: admit a closing JUMP onto a target token owned by the JitCellToken this compilation attaches to `_jump_to_existing_trace` walks `jitcelltoken.target_tokens` (unroll.py:171), so every candidate upstream offers belongs to one JitCellToken. pyre seeds the candidates from `compiled_loops[green_key].front_target_tokens`, a green-key side table that survives a recompile, so a candidate can name a token of an earlier, retired compilation. The unroll pass therefore discarded every close whose JUMP did not name the body token this compilation had just pushed. Carry the token the artifact will be installed under and admit a close onto any of its target tokens. `compile_retrace` resolves it from the source guard's own loop token, which is where `compile.py:797-811` attaches the result, so such a close stays inside one code buffer. A `compile_loop` has no such token yet — `compile.py:287-289` expresses the same thing by resetting `jitcell_token.target_tokens` to `[start_descr]`, leaving nothing matchable — so it admits nothing beyond its own body. `synth/retrace_outer_loop_type_flip` goes from `loops_aborted=2 retraces_compiled=0 guard_failures=590` to `loops_aborted=0 retraces_compiled=1 bridges_compiled=1 guard_failures=201` on all three backends. Assisted-by: Claude * jit: give the retrace path what unroll.py gives it — the bridge's runtime boxes, and neither the budget nor the disable sentinel from the loop path Three divergences from `optimize_peeled_loop` (unroll.py:112-180), which contains no `retraced_count` bookkeeping at all and calls `disable_retracing_if_max_retrace_guards` exactly once. The `force_boxes=true` retry at unroll.py:161-168 is unconditional. pyre had copied `optimize_bridge`'s accounting (unroll.py:213-226) onto the loop path, so every loop compile whose first match missed spent one unit of the per-JitCellToken retrace budget that upstream reserves for bridges. The budget's own increment lives on the bridge path (`tok.set_retraced_count(tok.get_retraced_count() + 1)`, unroll.py:213-215) and is untouched. The two arms differed only in that bookkeeping, so they collapse into one call. `disable_retracing_if_max_retrace_guards` ran before the close ladder, and a second time over the combined preamble+body list. Upstream runs it after both `jump_to_preamble` early returns — only for a loop that closed — over `self._newoperations`, the peeled body. Either write sets `retraced_count = u32::MAX`, which the early check in `compile_loop` reads as a permanent "skipping recompile" for that green key. `unroll.py:231-233` threads the bridge's own `runtime_boxes` into `ExportedState`, and unroll.py:153/166 pass `state.runtime_boxes` to `jump_to_existing_trace`. The only assignment in pyre was on the `compile_loop` branch, so `generate_guards` saw an empty list on every retrace and each runtime-guided arm — GUARD_VALUE, GUARD_NONNULL, GUARD_CLASS/GUARD_NONNULL_CLASS, the `IntBound::make_guards` fallback — declined for want of a value to read. The length-mismatch fallback in unroll.rs is left alone; this makes its "a trace with no recorded JUMP" comment true again. No jit-stats and no wallclock movement across the corpus: all three backends gate clean, and an in-place revert A/B on dynasm with the arms interleaved reads 0.31-0.33s reverted against 0.32-0.34s applied. `retrace_limit` defaults to 0 (`rpython/rlib/jit.py:595`), so the retrace legs stay dormant for code that does not raise it. Assisted-by: Claude * bench: re-record synth/unary_negative's wasm baseline `guard_failures` 13 -> 2 with `loops_compiled` unchanged at 2. Not this branch's doing: reverting this branch's three metainterp files and rebuilding the wasm module reads the same 2, so the move belongs to the base this branch was rebased onto — `jit: residualise the whole int-box tail, and decline a kept-stack branch holding a NULL ConstPtr` (#1131). `check.py` reports the drop as IMPROVED and refuses to pass until the baseline is recorded. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
…ss-block walk (#1159) * jit: print the orthodox list sub-walk decline pc under PYRE_FBW_DEBUG_ABORT `OrthodoxSubWalkTraceUnsupported` carries the pc but the three decline arms dropped it. The identifier the decline names elsewhere is a symbolic fnaddr minted in `pyre-jit-trace/build.rs`, so a runtime reverse-name registry has nothing to read; the pc is what pairs the decline with a bytecode offset. Assisted-by: Claude * intobject: trace w_int_new's allocation again, as wrapint does #1131 put a `dont_look_inside` boundary around the int box's allocating tail (`w_int_box_slow`) because the sub-jitcode walk declined on the `SyntheticTransparentCtor` the `malloc_typed` arm lowered to, and named its own exit condition: "Drop the boundary once the fusion resolves a vtable there." #1141 landed that resolution -- `fuse_boxing_alloc` now follows the header pointers across the block boundary each call ends -- and its comment records the condition met, "it does now fire here", while keeping the boundary. `wrapint` (`objspace/std/intobject.py:903-921`) carries no `@dont_look_inside`; its comment reads "this whole function is getting inlined into every caller", and it allocates with `instantiate(W_IntObject)` then `w_res.intval = x`, the alloc-then-init pair the rtyper lowers to `new_with_vtable` + `setfield_gc`. Put the `malloc_typed` arm back in `w_int_new` where the fusion rewrites it into that pair, and keep the collector-heap arm as `w_int_gc_alloc` behind its own boundary: that arm carries a blocker that is still real, the wasm backend not lowering the offset-0 `ob_type` store faithfully. `bench/synth/list_pop_append` does not decide this. It reads the same either way (2.6/2.3/2.6 against 2.5/2.5/2.3, three runs each), and #1141 records a negative control -- boundary removed with the fusion reverted -- that failed to reproduce the regression the boundary was added for, so the bench is uninformative in both directions. The trace shape is what differs and it is observable: with this change the compiled loop carries a `NewWithVtable` whose descr is `W_IntObject.intval` and no residual call to any `w_int_*` boxing symbol. A residual call can never be virtualized. check.py ALL PASSED on all three backends -- dynasm 417/417, cranelift 416/416, wasm 412/412 -- and cargo test --workspace green. The box is still not virtualized away, for a reason outside this change: `orthodox_list_append_commit` deliberately forces the value with a ptr->int->ptr identity pair so the descended sub-walk reads the current iteration's payload, and that force lands before the class guard that would otherwise fold. Assisted-by: Claude * majit: test fuse_boxing_alloc across the links a split cluster crosses `resolve_addr` steps through `Block.inputargs` and requires every predecessor to agree, but every `fuse_boxing_alloc` case built its cluster in a single block, so neither behaviour was reached by a test: the only case added with the walk is a decline that resolves inside one block. Four rows over the same one-payload `W_FloatObject` cluster, differing only in where the header values come from: one relay block between the producer and the ctor, two relay blocks, two predecessors of a merge block naming one type, and two naming different types. Each asserts the fused count, the address stamped on the `NewWithVtable`, and whether the `malloc_typed` survives as a residual. Measured by ablation on this tree: returning `None` for a phi fails the "one link crossing" row, and dropping the disagreement arm fails the "predecessors naming two types" row. The six pre-existing `fuse_boxing_alloc` tests pass under both ablations. Assisted-by: Claude
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.
Two independent JIT fixes, plus the baseline the second one moves.
1.
list.pop()'s fold comes back on the compiled loopw_int_gc_alloc(the collector-heap arm) was residualised; themalloc_typedfall-through next to it was not. That fall-through builds a
W_IntObjectonthe stack, which lowers to a
SyntheticTransparentCtorfor itsPyObjectheader whose funcptr constant degrades to a
symbolic_fnaddrhash. Adescending sub-jitcode walk cannot record such a call, so it declines the
entire descent — which is what took
list.pop()'s fold off the compiledloop, at pc=67 on
0x7add68d152cb6ceb.Fold both arms into one
w_int_box_slowand put thedont_look_insideboundary on it.
w_int_newkeeps its tagged and prebuilt-small-int fast pathsinside the trace.
bench/synth/list_pop_append:2. A kept-stack branch whose mirror holds a NULL
ConstPtrExtracted from #1118, which does not carry #1087 on its base and so cannot
land this on its own. Adds
kept_stack_has_null_const_slotand takes it asHazard (4) in
guarded_branch_core.The NULL
ConstPtrencoding is what a genuine null operand (PUSH_NULLaheadof a
CALL) and an unset vable shadow slot both decode to, so a keptoperand-stack slot holding one has no source the resume snapshot can name;
where the not-taken edge decodes no
ref_copymoves there is no fallbackeither, and the resume rebuilds the slot NULL.
re/_parser.py's_parse_subevaluates
not nested and not itemsinside_parse's argument list, so bothcalls'
PUSH_NULLslots are kept across the short-circuit guard andnested + 1arrives NULL.The decline is deliberately narrower than "the mirror does not cover" — a
mirror shorter than the resume depth, and a
NONEhole, both still resumethrough the shadow and must keep compiling; declining for those loses every
bridge in
bench/synth/attr_cache_invalidationand turns its 1002 guardfailures into 4 million.
test.test_regoes FAIL -> PASS on dynasm. The baseline re-record forbench/synth/list_append_write_barrier_gc(all three backends) is the cost:bridges_compiled5 -> 3,guard_failures1345 -> 938,loops_aborted1 -> 2,
loops_compiled12 -> 11.Verification
Three backends at base
1b31a5b3060, HEAD unchanged across each run:Both dynasm failures are accounted for.
list_append_write_barrier_gcranbefore the baseline update and reported exactly the four numbers now
committed (
observed loops_compiled=11 bridges_compiled=3); cranelift andwasm are green on that same baseline.
test.test_pickletoolsis a pre-existingmain failure from #1087, not this patch — it is fixed separately on the
perf-loopbranch.Known parity debt, documented not fixed
A Codex parity review flags two items in section 1. Both are real
classifications; neither is fixed here, and each carries a named blocker.
dont_look_insidearound the ordinary allocation is a deviation fromwrapint(objspace/std/intobject.py), which keeps its allocation inline.The orthodox lowering is
new_with_vtable, andfuse_boxing_allocis thepass meant to produce it — but instrumented over this tree it fires
nowhere: all 134 candidate sites report the vtable unresolved, because
resolve_vtable_addrreads aHostStaticAddrs.pytypesthat is empty in thebuild-script pipeline the pass runs in. The convergence path is recorded in
the doc comment on
w_int_box_slow.consume_boxes(resume.py) would restorewholesale upstream. It exists only because the mirror encodes "unset" and
"genuine null operand" identically; the fix is a distinguishable unset
encoding, after which the predicate can be deleted.
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Reliability
Benchmarks