jit: relocate a bridge's target tokens, and name the FOR_ITER deferred-admit call boundary - #1082
Merged
Conversation
WalkthroughThe JIT assemblers now validate cross-buffer jump targets and publish target tokens with ordered frame-depth updates. Deferred inline-call replay now uses explicit call-boundary classification. Two parity tests cover nested-loop retracing and deferred subscript inlining. Type lookup fast-path documentation was clarified. ChangesDynasm cross-buffer relocation
Deferred inline-call boundary handling
Type lookup path documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JITAssembler
participant ExternalJump
participant LoopOrBridge
participant ExecutableBuffer
JITAssembler->>ExternalJump: inspect cross-buffer target
ExternalJump-->>JITAssembler: return target address
JITAssembler->>LoopOrBridge: reject invalid target or continue assembly
LoopOrBridge->>ExecutableBuffer: finalize compiled code
ExecutableBuffer-->>LoopOrBridge: return executable address
LoopOrBridge->>JITAssembler: publish frame depth and target tokens
sequenceDiagram
participant Walker
participant InlineCall
participant DeferredReplay
Walker->>InlineCall: pass entry_is_call_boundary
InlineCall->>DeferredReplay: evaluate deferred replay admission
DeferredReplay-->>InlineCall: admit or reject inline call
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae1d05d950
ℹ️ 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 lookup_in_type_wtf8(metatype, name).is_some_and(|descr| is_data_descr(descr)) { | ||
| return None; | ||
| } | ||
| let w_value = lookup_in_type_wtf8(w_type, name)?; |
There was a problem hiding this comment.
When another thread mutates C.attr while either new type-attribute fold is being recorded, this lookup can read the old value before walker_pin_type_version_tag installs its watcher. Both callers discard the returned version_tag, so if the mutation lands between this read and watcher installation, the watcher captures the new version and optimizer validation succeeds while the old w_value is baked into the trace indefinitely. Pyre has no GIL, and state.rs:4475-4480 explicitly requires watcher installation before the protected read for this reason; install first or verify that the returned tag still matches after installation before emitting the constant. This otherwise makes the generated JIT observably diverge from interpreter attribute lookup.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/gate-triage.md`:
- Around line 840-846: Reconcile the default-OFF experiment totals in the triage
document: update the line-895 count and any derived totals to reflect that only
_CALLEE_VSTACK remains, keeping the surrounding retirement history and
adoption-target description consistent.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 4162-4171: Update the explanatory comment near
seed_standing_exception_for_walk to state that call_jit.rs publishes
BH_LAST_EXC_VALUE after start_bridge_tracing returns and before
seed_standing_exception_for_walk runs. Remove the inaccurate claim that
publication occurs before bridge tracing starts, while preserving the remaining
exception-arm and carrier-walk behavior descriptions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 31fdee86-a907-4934-97b9-6fdf3c6acea1
📒 Files selected for processing (13)
majit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/pickle_ctor_args.pypyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/type_dict_surrogate.pypyre/gate-triage.mdpyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rs
💤 Files with no reviewable changes (2)
- pyre/pyre-jit-trace/src/state.rs
- majit/majit-metainterp/src/trace_ctx.rs
Comment on lines
+4162
to
+4171
| // For the walks that reach here this cell is the delivery path for | ||
| // `_prepare_exception_resumption`: `call_jit.rs` publishes | ||
| // `cpu.grab_exc_value`'s result before bridge tracing starts, zeroes it when | ||
| // the guard carried no exception, and declines the bridge outright in the | ||
| // third case. On an exception-guard bridge one of the two arms below always | ||
| // returns, so a pre-seed placed on the sym in `setup_bridge_sym` could only | ||
| // rewrite the pointer this read applies anyway. Scoped to this leg: the | ||
| // multi-frame carrier walk never runs this function — `trace.rs` routes it | ||
| // through `drive_bridge_carrier_walk`, whose sub-walk seeds itself off | ||
| // `root_sym.last_exc_box()` instead. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the exception publication point accurately.
call_jit.rs publishes BH_LAST_EXC_VALUE after start_bridge_tracing returns. The value is available before seed_standing_exception_for_walk runs, not before bridge tracing starts.
Proposed wording
- // `cpu.grab_exc_value`'s result before bridge tracing starts, zeroes it when
+ // `cpu.grab_exc_value`'s result before the bridge bytecode walk starts, zeroes it when📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // For the walks that reach here this cell is the delivery path for | |
| // `_prepare_exception_resumption`: `call_jit.rs` publishes | |
| // `cpu.grab_exc_value`'s result before bridge tracing starts, zeroes it when | |
| // the guard carried no exception, and declines the bridge outright in the | |
| // third case. On an exception-guard bridge one of the two arms below always | |
| // returns, so a pre-seed placed on the sym in `setup_bridge_sym` could only | |
| // rewrite the pointer this read applies anyway. Scoped to this leg: the | |
| // multi-frame carrier walk never runs this function — `trace.rs` routes it | |
| // through `drive_bridge_carrier_walk`, whose sub-walk seeds itself off | |
| // `root_sym.last_exc_box()` instead. | |
| // For the walks that reach here this cell is the delivery path for | |
| // `_prepare_exception_resumption`: `call_jit.rs` publishes | |
| // `cpu.grab_exc_value`'s result before the bridge bytecode walk starts, zeroes it when | |
| // the guard carried no exception, and declines the bridge outright in the | |
| // third case. On an exception-guard bridge one of the two arms below always | |
| // returns, so a pre-seed placed on the sym in `setup_bridge_sym` could only | |
| // rewrite the pointer this read applies anyway. Scoped to this leg: the | |
| // multi-frame carrier walk never runs this function — `trace.rs` routes it | |
| // through `drive_bridge_carrier_walk`, whose sub-walk seeds itself off | |
| // `root_sym.last_exc_box()` instead. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs` around lines 4162 - 4171,
Update the explanatory comment near seed_standing_exception_for_walk to state
that call_jit.rs publishes BH_LAST_EXC_VALUE after start_bridge_tracing returns
and before seed_standing_exception_for_walk runs. Remove the inaccurate claim
that publication occurs before bridge tracing starts, while preserving the
remaining exception-arm and carrier-walk behavior descriptions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f9de53c92
ℹ️ 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".
| dst: usize, | ||
| dst_bank: char, | ||
| ) -> Result<Option<()>, DispatchError> { | ||
| if !ctx.is_authoritative_executor || dst_bank != 'r' { |
There was a problem hiding this comment.
When an inlined callee performs a side effect before C.attr, this helper now folds the read even in inline_subwalk mode and emits receiver/version guards whose snapshots resume at the caller's CALL boundary. If the receiver changes or the type is mutated after the trace becomes hot, the guard fails after the earlier effect has executed and the interpreter reruns the callee from entry, applying that effect twice. The neighboring classmethod fold explicitly declines this mode for this reason; add the same restriction here and to try_walker_specialize_builtin_type_getattr, or preserve a per-callee resume frame before allowing these guards.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d6fcf646
ℹ️ 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 lookup_in_type_wtf8(metatype, name).is_some_and(|descr| is_data_descr(descr)) { | ||
| return None; | ||
| } | ||
| let w_value = lookup_in_type_wtf8(w_type, name)?; |
There was a problem hiding this comment.
__annotate_func__
When a base class defines __annotate_func__ but a subclass does not, this MRO lookup returns the base's value and both new folds bake it into the trace. The interpreter's special object_getattr_miss branch deliberately checks only the receiver class's own __annotate_func__, then its own __annotate__, and otherwise returns None, because this annotation metadata is not inherited. Consequently, a hot C.__annotate_func__ or getattr(C, "__annotate_func__") can change from None while interpreted to the base value after JIT compilation; decline this name or mirror the own-dictionary lookup.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/1393fd5c17050fd56182f9ab5d27b48fe9a90442/pyre-jit/src/call_jit.rs#L2270
When a guard failure resumes an inlined callee that subsequently propagates an exception, this branch removes the frame from topframeref but never sets FLAG_FRAME_FINISHED. The interpreter sets that flag before propagating from a frame (eval.rs:1921), and the tracing-side walker_ec_leave does likewise; without it, the traceback exposes an already-unwound frame whose frame.clear() incorrectly raises RuntimeError("cannot clear an executing frame"). Set the finished state before leaving when got_exception is true.
AGENTS.md reference: AGENTS.md:L14-L20
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/d24b612922c4324dedf0a5a3bfc7692db0a0544f/pyre-jit/src/call_jit.rs#L2634-L2635
When a pending guard exception escapes a resumed callee, this call removes the callee from ec.topframeref, and release_bh_rd immediately clears the blackhole registers before the downstream traceback recorder uses frame_ptr. For an otherwise-unescaped inlined frame, that leaves only a bare Rust pointer while record_application_traceback allocates; pytraceback.rs:164-177 explicitly requires callers to keep the frame reachable across that allocation. A collection there can reclaim the frame and leave the traceback with a dangling pointer. Record the traceback before leaving, as the sibling bh.got_exception path does, or pin the frame across this interval.
AGENTS.md reference: AGENTS.md:L14-L20
ℹ️ 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 4a3ab98). 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 7, 2026
…#1082) (#1101) * jit: restore the BUILD_TUPLE fold order and the arity-2 decline's wording `try_walker_specialize_newtuple` runs before `try_walker_specialize_newtuple_object` again, and the two dispatch comments in `residual_call` go back to describing that order. With arity 2 declined in the canonical fold the two arms cover disjoint arities, so the order no longer decides which one claims a pair. `try_walker_specialize_newtuple_object`'s header and the concrete-shadow comment go back to naming arity 2 as declined. A paragraph records what a side exit does when the decline is lifted: the trace hands a real pair, with inline `value0` / `value1` and no `wrappeditems` block, to a consumer picked for the canonical layout, and `try_walker_specialize_subscr_specialised_pair` reads a field that is not there. Assisted-by: Claude * jit: publish the inline resume for a single-callee vable promote guard `walker_capture_inline_nonstandard_vable_guard` published the callee's own resume coordinate only when the paused-caller chain covered the full inline depth (`n_parents > 0 && n_parents == n_callees`). A chain of one callee frame and no paused caller is covered by the frame list just as directly, so it takes the same publish; every other shape still falls through to the single-frame sentinel. `walker_capture_multi_frame_inline_snapshot` reaches `publish_outermost_parent_vable_scalars` with an empty `parent_frames` on that new shape, so the call is made only when there is a parent to publish. Assisted-by: Claude * builtins: dispatch `type.__new__` on the argument count `type_descr_new` located `(name, bases, dict)` by scanning the positional arguments for the first `str` with two more behind it, and took the metatype as the last preceding type. `descr__new__` (typeobject.py:886-911) keys the one-versus-three form on `len(__args__.arguments_w)` instead, runs `_precheck_for_new` unconditionally after that decision, and only then reads the name. The scan reached `type_descr_new_with_metaclass` before the branch that prechecks, so a non-type metatype was dropped rather than refused and `type.__new__(42, 'A', (), {})` built a class. The scan's header cited a `[self, metatype, name, bases, dict]` shape from `super()`. No receiver is prepended: `super().__new__(mcls)` inside a metaclass raises `M.__new__() takes exactly 3 arguments (1 given)`, which is the one-element `pos` arm, and `super().__new__ is type.__new__` holds with `__self__` the type. `pos[0]` is always the metatype. `pos` being empty is a shape upstream's gateway cannot produce, since it supplies `w_typetype` from a declared parameter; it is refused here in `tp_new_wrapper`'s words rather than through the three-argument message. `_calculate_metaclass` (typeobject.py:945) now receives the bases as written. `(object,)` is substituted for an empty tuple in `W_TypeObject.__init__` (`bases_w or [space.w_object]`), after the winner is settled; supplying it earlier weighed an explicit metatype against `type(object)` and reported a metaclass conflict for `type.__new__(int, 'A', (), {})`. The winner then passes `check_user_subclass` (typeobject.py:555-567), which `allocate_instance` runs on the way in and which is what names `int is not a subtype of type`. `_ast`'s heap-type creation passed `[name, bases, dict]` and relied on the scan matching at index 0; it now passes the metatype. The `_ctypes` metaclasses already forwarded `[metatype, name, bases, dict]`. `bool.__new__`'s over-arity refusal reported `got more`, the only such wording in the tree; the gateway counts the class argument along with the value. Assisted-by: Claude * str: decode a buffer `ljust` / `rjust` fill operand `pad_fillchar` refused every non-`str` fill, and its own header recorded the gap: "The decode itself is not imported". `descr_ljust` and `descr_rjust` (unicodeobject.py:1352,1371) convert the operand with `convert_arg_to_w_unicode` (unicodeobject.py:175-184), which declines `bytes` by name and hands everything else to `decode_object` (unicodeobject.py:1727-1739); that reads the operand as a buffer and decodes it strict UTF-8, so a `bytearray` or `memoryview` becomes a fill character. `descr_center` (unicodeobject.py:1101) reads its operand with `space.utf8_w` and is unchanged. The `"decoding to str: ..."` arm now reports only an operand that exports no buffer at all, which is what `decode_object` maps the buffer error to. A buffer whose bytes are not valid UTF-8 raises from the decode. The single-character check applies to the decoded operand, so a multi-byte fill is rejected by its code-point count. Assisted-by: Claude * jit: trace exact scalar string helpers * jit: delete the PYRE_CARRIER_EXC_RESUME gate and the bridge-sym exception pre-seed it guarded The gate was default-off, so the block it guarded never executed in any production run, CI run, or recorded baseline. What is removed is an unattested ON path for a slice that was never built. On the single-frame leg the pre-seed was redundant. `trace_and_compile_from_bridge` publishes `cpu.grab_exc_value`'s result into `BH_LAST_EXC_VALUE` before bridge tracing starts, zeroes it when the guard carried no exception, and declines the bridge in the third case; `seed_standing_exception_for_walk` then runs at walk start, after `setup_bridge_sym`, and for an exception-guard bridge returns from one of its first two arms in every case, overwriting all five exception slots or clearing them. It has no `last_exc_box.is_none()` short-circuit, so it always wins. Measured with a probe at both seed sites over 369 synth benches: 23 exception-guard bridges in 14 benches, 7 of which the gate would have seeded, and in all 23 the value at the walk seed equalled the value at the setup seed, pointer for pointer. `PYRE_CARRIER_EXC_RESUME=1` over the whole corpus was dynasm 386/386, byte-identical to gate-off, with no jit-stats movement. The redundancy does not extend to the multi-frame carrier leg, which never runs the walk seed: `setup_bridge_sym` installs the inline carrier whenever `resume_data.frames.len() > 1`, `trace_bytecode` returns through `drive_bridge_carrier_walk` before the full-body-walk leg, and `drive_bridge_frame_subwalk` seeds its sub-walk off `root_sym.last_exc_box()`. That leg is unexercised by bench/synth — all 23 bridges took the single-frame leg — which is why the gate was never validated. gate-triage.md §1b records it, and records that a rebuilt slice must seed from `BH_LAST_EXC_VALUE`. `TraceCtx::bridge_guard_exc` and the `guard_exc` parameter of `start_bridge_tracing` go with it: the deleted line was their only reader. `guard_exc` stays live in call_jit.rs for the `BH_LAST_EXC_VALUE` publish, `pending_exc`, `GuardExcRoot::park` and the blackhole resume. gate-triage.md: §1b rewritten to retire the gate, keeping §1e's 732,660/170 census and its seven named benches as history and correcting the "reachable, not inert" inference — the census instruments `handle_fail`, one layer above the seed site. The booked `class_of_last_exc_is_const` delta is withdrawn as miscited: `prepare_resume_from_failure` calls `handle_possible_exception` three lines later (pyjitpl.py:3169), which ends `class_of_last_exc_is_const = True` (pyjitpl.py:3416), so upstream's post-resumption steady state is `True` too. §3 gains a row, Dead 12 -> 13, and the default-OFF experiment count goes 2 -> 1. check.py dynasm 387/387, cranelift 387/387, both after re-extracting LLBC. Local macOS wasm shows four benches off by one or two `guard_failures`; CI at the parent commit reports wasm 383/383, so those are left for CI to arbitrate. Assisted-by: Claude * jit: fold a type-attribute read to a constant at record time `getattr(C, name)` and `C.attr` each residualized as one opaque CallMayForce. Its descr is unregistered, so the optimizer assumed the most pessimistic effect tier and mandated a ForceToken plus two SetfieldGc and a GuardNotForced on every iteration -- even though all four call arguments were already Const. Add `type_attr_value_fast_path`, which admits only the typeobject.py:811-828 shape whose result is a class-namespace value returned unchanged: a cacheable type receiver whose metaclass is exactly `type`, no metatype data descriptor for the name, a class-MRO hit, and a value whose type is a non-heap builtin defining no `__get__`. The name is taken as `&Wtf8` and resolved through `lookup_in_type_wtf8`, so a lone or embedded surrogate takes the same path. Two walker folds consume it: `try_walker_specialize_builtin_type_getattr` (PyreHelperKind::CallFn) and `try_walker_specialize_load_type_attr` (PyreHelperKind::LoadAttr). Each pins the callable, the receiver, the name object and the receiver's version tag before writing the value as a green constant. The version pin is a quasi-immutable watcher, so it emits no per-iteration op, and `mutated` recurses into subclasses, so it also covers a write to any base class. `is_builtin_getattr_function` recognizes the callable through the shared `is_builtin_code_function` identity test. Marginal cost of one loop-invariant read, dynasm, against an in-place revert control arm (empty-loop baseline 5.6ns folded, 6.1ns on the control arm): `C.attr` 144.7 -> 6.0ns, `getattr(C, n)` 269.8 -> 5.8ns, an embedded surrogate 1034.5 -> 8.7ns, a lone surrogate 1213.4 -> 6.3ns. Every folded reading is at the empty-loop floor. The control-arm readings were taken with sibling builds on the box and are inflated in absolute terms; the separation is not. They were measured one base earlier and were not re-run here. bench/synth/type_dict_surrogate: max-pypy-ratio 350 -> 21. Twelve local readings of the folded fixture across the three backends span 3.4x-7.0x, and the ceiling is three times the slowest; the derivation is in the header. bench/synth/pickle_terminal_raise_resume: re-record the wasm baseline, loops_aborted 13 -> 14 and loops_compiled 67 -> 66 -- one loop in the fixture aborts where it used to compile. The bench's output is unchanged, its perf gate is unaffected, and dynasm and cranelift are unchanged. Assisted-by: Claude * jit: leave the resumed callee's execution-context scope in the blackhole A guard-failure blackhole that resumes inside an inlined callee transfers the callee's return value to its caller and releases the callee interpreter, but releasing a BlackholeInterpreter does not restore `topframeref`. The completed callee stayed the current frame, so any later read of the caller's locals selected it instead -- and an inlined callee's frame carries only its parameters. JIT-only, and identical on all three backends; CPython, pypy3 and `PYRE_NO_JIT=1` agree with each other: def inner(k): if k == 2000: sys._getframe(1) # one bare force, reads nothing return k def outer(n): base = 11 acc = 0 for i in range(n): acc += inner(i) & 7 return sorted(locals().keys()) reads `['k']` instead of `['acc', 'base', 'i', 'n']`, and the same shape makes `sys._getframe(1).f_locals['base']` raise KeyError. One bare `sys._getframe(1)` is enough -- no attribute of the frame has to be read, and the caller observes it in its own `locals()` with no `sys._getframe` at the read. `leave_resumed_blackhole_frame` performs the `executioncontext.py:91-107 leave` frame transition before the innermost blackhole is released, guarded on `topframeref` still resolving to that frame, and takes `got_exception` from the call site: the two exception-propagation paths pass true and the return-value transfer passes false. It forces no vref. The guard already establishes that the outgoing vref resolves to this frame, so `leave`'s own `frame_vref()` has nothing left to materialize, and the caller is reached through `vref_referent` rather than `get_f_back`. Forcing there panicked synth/exception_escape_inlined_midframe_tb_node on wasm with `InvalidVirtualRef: frame-chain vref forced after its frame died`, because the compiled frame is already gone by this point. bench/synth/getframe_caller_locals_after_resume: new. It asserts the `f_locals` read and the caller's own `locals()` together, since either can hold while the other breaks, and it needs two triggering calls -- a single one passes with or without the change. max-pypy-ratio is three times the slowest of nine local readings across the three backends, which span 1.8x-5.5x. check.py: dynasm 392/392, cranelift 392/392, wasm 388/388. Assisted-by: Claude * jit: publish the merge point's own stack depth when a closing JUMP crosses bytecode offsets pyre compiled no retrace at all. A 3-backend MC_DIAG census over 397 synth fixtures left one refusal cluster, identical on dynasm, cranelift and wasm and confined to `synth/retrace_accumulator_type_flip`: five `cb_retrace_req`, five `wct_declined`, five `ct_compile_bridge_false`, five `close_hdr_fallback` and five `retrace_arity_giveup`. `retrace_limit` defaults to 0 (`rpython/rlib/jit.py:595`), so that fixture — which sets it to 5 — is the only one in the corpus that reaches `compile_retrace`. pypy3 on the same file compiles the retrace: under a "bridge out of Guard 0x..." header it emits a float-specialised `label(p0, p11, p5, p6, f24, i26, i16)` and closes onto it. `jit-summary` still reads "loops: 1 / bridges: 1" there, so the summary does not report a retrace; the label inside the bridge artifact does. `close_loop_args_at` publishes a stack depth for the duration of the JUMP-arg derivation, and `merge_point_stack_depth_to_recover` supplied it only when the merge point's static depth EXCEEDED the depth the frame still advertises. This retrace resumes from a guard at one bytecode offset and closes onto the loop header at another, where the static depth is shallower — 3 against the resumed frame's 5 — so the helper declined and `flush_to_frame` pinned the stale 5 as a constant. `valuestackdepth` is virtual-state index 4 of the `[frame, ec, last_instr, pycode, valuestackdepth, ...]` closing-JUMP layout, so the retrace then failed to match even the target token it had just created: [jit][jte] virtualstate mismatch index=4 box=ConstInt(5) expected=... Constant(Int(3)) ... incoming=... Constant(Int(5)) ... [jit][jte] target_token #2 generate_guards failed (force_boxes=false) Only after that did `jump_to_preamble` run, and its arity give-up (MC_DIAG slot 57) refused the 13-arg body JUMP against the 8-arg preamble LABEL. That give-up is the symptom, not the cause. A bytecode offset has exactly one operand-stack depth, so the helper now takes the target's depth in either direction. Narrowing loses nothing: JUMP args are sized by `target_array_capacity` — the full virtualizable array (`nlocals + ncells + co_stacksize`) — while the published depth only sets the live prefix, and slots above it are null-padded capacity that `materialize_fail_arg_slot` fills from `concrete_value_at`. The comment that claimed narrowing "would lose a value the JUMP must carry" was wrong about its own consumer, and is corrected along with the unit test, which now covers deep-start/shallow-target as well as the reverse. bench/synth/retrace_accumulator_type_flip: `loops_aborted` 5 -> 0 and `guard_failures` 1202 -> 202 on all three backends; `loops_compiled` and `bridges_compiled` stay 1 and 1, because the counters class the attached retrace as that bridge. Its header is rewritten: the fixture used to document the give-up it reached, and now documents the assembled retrace and the cross-offset closure that produces it. check.py: dynasm 392/392, cranelift 392/392, wasm 388/388. Assisted-by: Claude * check.py: gate an assembled retrace on its own counter The retrace the previous commit made pyre assemble was observable in jit-stats only as `loops_aborted` 5 -> 0 — an absence. A later change that stops pyre REQUESTING the retrace leaves `loops_aborted` at 0 and LOWERS `guard_failures`, and `_jit_stats_change` classes a `guard_failures` fall as IMPROVED, so the corpus's only retrace coverage would be re-recorded away without anyone being told. `retrace_limit` defaults to 0 (`rpython/rlib/jit.py:595`), so `synth/retrace_accumulator_type_flip` is the only bench that reaches `compile_retrace` at all and there is no second witness to notice. `retraces_compiled` is bumped where a retrace is attached, next to the `attached retrace to guard` log, so it counts an assembled artifact rather than a request or a give-up. It is emitted by both `[jit-stats]` surfaces — `pyrex` for the native backends and `pyre-wasm-runner` for wasm, the latter through a `pyre_jit_retraces_compiled` guest export, since the host resolves each counter by name and reports a missing one rather than reading it as zero. check.py gates it on a FALL, the polarity `loops_compiled` already carries: the regression is the artifact ceasing to exist. `_jit_stats_change` reads a field absent from either side as "0", so the other 391 baselines gate on the new counter without being re-recorded; only the one bench whose value is non-zero is re-recorded, on all three backends. Both directions of the counter were checked against the built binary rather than assumed: `retrace_accumulator_type_flip` reports `bridges_compiled=1 retraces_compiled=1`, while `bench/fannkuch` reports `bridges_compiled=22 retraces_compiled=0` and `bench/nbody` `bridges_compiled=5 retraces_compiled=0` — twenty-seven ordinary bridge attachments move the counter not at all. Each backend's suite reported the `retraces_compiled 0 -> 1` gate itself, wasm included, which is what shows the export reaches the host. check.py: dynasm 391 passed, cranelift 391 passed, wasm 387 passed, each with the single expected `retraces_compiled 0 -> 1` re-record. Assisted-by: Claude
The folds themselves landed in #1101; what is left of this commit is the cross-reference between them. `type_name_obj_fast_path` / `try_walker_specialize_load_type_name_attr` cover typeobject.py:814-819, the metatype-data-descriptor arm that `__name__` resolves through. `type_attr_value_fast_path` / `try_walker_specialize_load_type_attr` cover :820-822, the class-MRO-value arm, and their oracle refuses any name the metatype answers with a data descriptor. Each doc comment now names its own arm and points at the other, so a reader landing on either knows the two are disjoint rather than redundant. Assisted-by: Claude
…P to an unrelocated target `assemble_bridge` never ran the `compiled_target_tokens` relocation that `assemble_loop` inlined, so a LABEL assembled inside a bridge left its `ll_loop_code` holding the in-buffer offset it was given at emission. A retrace is attached to a guard, so it is assembled through the bridge path and carries its own LABEL; a later trace closing onto that token missed `target_tokens_currently_compiling`, read the field, and baked the offset as an absolute branch target — `br x16` with 0x1fc, a deterministic SIGSEGV on a nested loop whose inner accumulator changes type. Extract the relocation into `fixup_target_tokens` and call it from both assemble paths, matching `aarch64/assembler.py:1087` (called at `:98` and `:213`) and `x86/assembler.py:990` (`:612`, `:706`). Both dynasm backends had the omission; cranelift publishes an absolute address instead and is unaffected. The list is consumed, so a token cannot be relocated twice. A cross-buffer JUMP whose target is below the first page now fails the compile rather than emitting the branch. Dynasm bakes the immediate at codegen, so neither 0 (target never compiled) nor an unrelocated offset can be repaired later; both are always wild branches. Add `jit_retrace_nested_loop_bridge_label`, which SIGSEGVs before this change. Assisted-by: Claude
…of inferring it
The `CalleeReplaySafety::DeferredCall` arm admitted a callee when
`arg_class_guard.is_none()`, standing in for "the entry is a CALL the abort
rewind can name". The arm's comment justified the proxy by asserting a binop
dunder dispatch was the only entry carrying an `arg_class_guard`. It is not:
`try_walker_inline_subscr_getitem` enters from `BINARY_OP` and passes no
`arg_class_guard`, so `obj[key]` was admitted and resumed one operand short —
the subscript index was replaced at runtime by an unrelated live Ref, failing
`test.test_re` in CI with
TypeError: list indices must be integers or slices, not list_iterator
`try_walker_inline_resolved_user_call` now takes an explicit
`entry_is_call_boundary`, and the arm reads it directly. Audited per call site
against the old predicate's value: `try_walker_inline_user_binop` (BINARY_OP)
and `try_walker_inline_user_compareop` (COMPARE_OP) already evaluated to
`false` and keep it; `try_walker_inline_subscr_getitem` changes from `true` to
`false` and is the only behaviour change. `try_walker_inline_property_get`,
`try_walker_inline_property_set` and
`try_walker_specialize_seqiter_getitem_next` enter from LOAD_ATTR, STORE_ATTR
and FOR_ITER rather than a CALL but stay `true`, which is what they evaluated
to before; each says so at its call site.
What the gate asks is the entry opcode's stack effect, not the spelling: an
entry that only peeks re-executes from the stack the rewind already has. The
FOR_ITER call site carries that reason now rather than a reference to the
property routes — `opcode_for_iter` peeks its single iterator operand where
`opcode_binary_op` pops both of its own. The two attribute entries reach
their stack discipline through `load_attr_cached` / `store_attr_cached`, which
this audit did not follow, so they keep the standing they had and say so.
Add `jit_subscr_getitem_inline_keeps_its_index`, which raises that TypeError
before this change and is JIT-only.
Assisted-by: Claude
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 5479-5481: Set entry_is_call_boundary to false for both
attribute-entry sites in pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
at lines 5479-5481 and 5588-5589. Update the LOAD_ATTR and STORE_ATTR handling
so deferred replay cannot admit these entries until the resume image preserves
their consumed operands.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ca6fbbf-30c4-4a3d-a9a9-6ec14c7cc8cc
📒 Files selected for processing (7)
majit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/x86/assembler.rspyre/extra_tests/parity_tests/jit_retrace_nested_loop_bridge_label.pypyre/extra_tests/parity_tests/jit_subscr_getitem_inline_keeps_its_index.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
Comment on lines
+5479
to
+5481
| // LOAD_ATTR is not a CALL either, but this entry is left admitted | ||
| // as it was: only the subscript one below has a witness. | ||
| true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject deferred replay for attribute entries.
LOAD_ATTR consumes its object operand. STORE_ATTR consumes its object and value operands. Neither entry can safely re-execute after a deferred abort from the preserved stack.
Passing true can admit a DeferredCall during FOR_ITER. A later abort can replay the attribute opcode without its original operands and corrupt execution state. Pass false at both sites until the resume image preserves the required operands.
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L5479-L5481: changeentry_is_call_boundarytofalse.pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L5588-L5589: changeentry_is_call_boundarytofalse.
Proposed fix
- true,
+ false,📍 Affects 1 file
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L5479-L5481(this comment)pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L5588-L5589
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 5479 -
5481, Set entry_is_call_boundary to false for both attribute-entry sites in
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs at lines 5479-5481 and
5588-5589. Update the LOAD_ATTR and STORE_ATTR handling so deferred replay
cannot admit these entries until the resume image preserves their consumed
operands.
youknowone
added a commit
that referenced
this pull request
Aug 9, 2026
`Seq.__getitem__` reached as `p[len(EMPTY)]`, so the index arrives on the operand stack rather than from a constant or a local, with an `isinstance(index, slice)` branch in the body so the inline has a residual to abort on. Prints 276000 under cpython, pypy3 and pyre. The defect the shape covers — the FOR_ITER deferred admission reading `arg_class_guard.is_none()` as a proxy for "the entry opcode is a CALL", which admitted the BINARY_OP-entered subscript inline and let the flush resume one operand short — is fixed in #1082, which names the property directly and carries its own parity test. This holds the shape under the jit-stats gate too. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 9, 2026
`Seq.__getitem__` reached as `p[len(EMPTY)]`, so the index arrives on the operand stack rather than from a constant or a local, with an `isinstance(index, slice)` branch in the body so the inline has a residual to abort on. Prints 276000 under cpython, pypy3 and pyre. The defect the shape covers — the FOR_ITER deferred admission reading `arg_class_guard.is_none()` as a proxy for "the entry opcode is a CALL", which admitted the BINARY_OP-entered subscript inline and let the flush resume one operand short — is fixed in #1082, which names the property directly and carries its own parity test. This holds the shape under the jit-stats gate too. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 9, 2026
`Seq.__getitem__` reached as `p[len(EMPTY)]`, so the index arrives on the operand stack rather than from a constant or a local, with an `isinstance(index, slice)` branch in the body so the inline has a residual to abort on. Prints 276000 under cpython, pypy3 and pyre. The defect the shape covers — the FOR_ITER deferred admission reading `arg_class_guard.is_none()` as a proxy for "the entry opcode is a CALL", which admitted the BINARY_OP-entered subscript inline and let the flush resume one operand short — is fixed in #1082, which names the property directly and carries its own parity test. This holds the shape under the jit-stats gate too. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 10, 2026
`Seq.__getitem__` reached as `p[len(EMPTY)]`, so the index arrives on the operand stack rather than from a constant or a local, with an `isinstance(index, slice)` branch in the body so the inline has a residual to abort on. Prints 276000 under cpython, pypy3 and pyre. The defect the shape covers — the FOR_ITER deferred admission reading `arg_class_guard.is_none()` as a proxy for "the entry opcode is a CALL", which admitted the BINARY_OP-entered subscript inline and let the flush resume one operand short — is fixed in #1082, which names the property directly and carries its own parity test. This holds the shape under the jit-stats gate too. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 10, 2026
…ts, and a measured FOR_ITER gate widening (#1103) * list: give the unused typed strategy an empty array, not a block `build_list_storage` called `IntArray::from_vec` and `FloatArray::from_vec` unconditionally, and `try_alloc_typed_items_block` clamps `cap` to 1 into the old-gen `try_gc_alloc_stable_raw`, so every list allocated two blocks whose strategy never reads them. The trace emitters leave those fields null: `emit_empty_list_inline` and `emit_object_list_inline` set only `length` / `items` / `strategy`, and `emit_typed_list_inline` writes one typed pair. Add `IntArray::empty()` / `FloatArray::empty()` and use them where emptiness is statically known — `build_list_storage`'s non-matching arms, `switch_to_object_strategy`, `w_list_clear`. `switch_to_correct_strategy` keeps `from_vec`, since its twin `emit_promote_empty_list_inline` emits a capacity-1 block and seeds the capacity getfield cache with 1. `base()` takes `wrapping_add`, so the null block yields the items offset — a non-null, 8-aligned address `from_raw_parts` accepts at length zero. `list_object_custom_trace` skips the ownership query on a null typed block. Assisted-by: Claude * optimizeopt: answer an unwritten field of a virtual with its typed zero virtualize.py:184-190 optimize_GETFIELD_GC_* resolves a field the virtual has never been written to through optimizer.new_const(fielddescr). Pyre carried only the written-field arm, so such a read fell through to OptEarlyForce, which forces every argument of a non-exempt operation and materialised the struct along with everything its fields reach. The array counterpart was already in place: NEW_ARRAY_CLEAR seeds every slot with the typed zero at creation (virtualize.py:27-35, info.py:507-514). typeptr keeps its own arm. heaptracker.py:66 excludes it from the virtual field set and the block above answers it from the descr vtable, so a struct whose descr carries no vtable must not fold its class pointer to null. Assisted-by: Claude * mapdict: keep builtin storage on user subclasses Restore exact int and bool objects to 24 bytes, Unicode objects to 64 bytes, and tuple objects to 40 bytes. Add distinct user-subclass layouts carrying mapdict map and storage fields, with their own GC types and traces. Select the wider layouts from builtin subclass constructors and resolve mapdict field descriptors from each concrete carrier layout. Keep the specialized attribute load guarded by the subclass map and storage descriptors. Record the wasm guard-count changes caused by the restored exact-object heap trajectory. Assisted-by: Claude * jit: record integer zero-divisor raising arms Assisted-by: Claude * mapdict: harden builtin subclass carriers Guard the live Python class before native mapdict field access. Size map descriptors to the target word and exclude specialised tuple layouts. Mark private user layouts as GC objects without adding duplicate subclass-range peers. Re-root every mapdict carrier on class reassignment and allocate hasdict structseq values with tuple-user storage. Extend parity coverage for exact-value exits, descriptors, slots, GC inspection, and structseq extras. Assisted-by: Claude * jit: initialize inline allocation scalar fields Assisted-by: Claude * jit: record range zero-step raising arms Assisted-by: Claude * jit: record float zero-divisor raising arms Assisted-by: Claude * jit: record bigint zero-divisor raising arms Assisted-by: Claude * jit: record negative bigint shift raising arms Assisted-by: Claude * jit: scope FOR_ITER safety to escaping range loops Assisted-by: Claude * bench: re-record seven wasm jitstats baselines on the rebased base Five of them (`exception_traceback_loop_forms`, `gc_bug_bridge_flavor_traceback_names`, `loops_comprehension`, `newslice_step_hot`, `unpack_ex_hot`) return to the values already committed on the base; the rebase conflict resolution had kept this branch's older measurements over them. Their only remaining difference from the base is added counter keys. `range_ctor_in_loop` compiles and enters its loop for the first time, so loops_compiled 1 -> 5, bridges_compiled 0 -> 3 and guard_failures 0 -> 1009: a fixture that never entered compiled code reported zero guard failures trivially. An in-place revert of the FOR_ITER admission reproduced the old values. `closure_per_call` guard_failures moves 420 -> 417. This one is not attributed by a control arm. Assisted-by: Claude * jit: diagnose FOR_ITER gate opcode declines Assisted-by: Claude * jit: gate FOR_ITER decline census allocation Assisted-by: Claude * jit: gate FOR_ITER decline census collection Assisted-by: Claude * jit: guard numeric binary specialization classes Assisted-by: Claude * jit: retain context on specialized builtin raises Assisted-by: Claude * jit: skip redundant numeric class guards Assisted-by: Claude * test: drive the numeric subclass fixture through the specialized pc The fixture fed its subclass operand to a tail expression at a different BINARY_OP pc than the loop that went hot, so that site was never specialized and the check passed on a binary without the class guards. Iterate a list whose tail holds the subclass instead, so it arrives at the pc under test. Adds left-operand cases and a bool-driven case, which reaches the tagged and bool path where walker_numeric_builtin_class returns null and no class guard is emitted at all. Assisted-by: Claude * Grow FOR_ITER regions through handler rejoins Assisted-by: Claude * Tighten escaping range append recognition Assisted-by: Claude * Update range constructor loop jitstats Assisted-by: Claude * jit: admit LIST_EXTEND in FOR_ITER bodies Assisted-by: Claude * jit: admit call-bearing LIST_APPEND bodies in the FOR_ITER gate A LIST_APPEND body was admitted only when the body performed no call, because a mid-body abort after the append routed through fbw_foriter_inflight_take, which refuses delivery and dropped the iteration's item. range_ctor_in_loop goes from mc_entered=0 to 813. The surrounding comment previously cited blackhole.py as authority for the append being rolled back and replayed once. It is not: blackhole.py:1712 is setposition, which continues from the coordinate already reached, and upstream places the resume coordinate past a residual call so the effect is never re-executed. State instead what pyre actually relies on, and record that a non-committed walk exit still keeps the legacy entry replay whose delivery can be refused. Assisted-by: Claude * jit: census in-flight FOR_ITER delivery outcomes Assisted-by: Claude * bench: re-record nineteen jit-stats baselines Five fixtures enter compiled code where they previously did not, so their zero counters were zero trivially: exception_group_type loops_compiled 0 -> 1, guard_failures 0 -> 1 list_append_virtual_payload loops_compiled 0 -> 2, bridges 0 -> 8, guard_failures 0 -> 1603 minmax_key_rooting loops_compiled 1 -> 2, bridges 0 -> 2, guard_failures 5 -> 409 range_ctor_in_loop loops_compiled 1 -> 3, bridges 0 -> 3, guard_failures 0 -> 811 (812 on wasm) global_store_plain_dict_globals (wasm) loops_compiled 5 -> 6, loops_aborted 1 -> 2, guard_failures 1 -> 18 pickle_terminal_raise_resume (wasm) loops_compiled 67 -> 68, loops_aborted 13 -> 14, guard_failures 339 -> 356 mapdict_frozen_unboxing_fold takes guard_failures 2 -> 8 with loops_compiled unchanged at 3. An A/B across the call-bearing LIST_APPEND admission alone gives mc_entered 2 -> 8 on the same fixture, so the counter tracks compiled-code entries one for one: its `[C(i) for i in range(n)]` comprehension is a call-bearing LIST_APPEND body. gc_bug_bridge_flavor_traceback_names (wasm) improves guard_failures 2027 -> 1670. exception_reused_object_tb_not_doubled (wasm) loses fbw_blackhole_adopted_single_frame 3 -> 0. A binary built from 128590c with no branch commits applied reports 0 on the same fixture, so the fall is the base's; the baseline was last recorded at 779da08. Every other counter on that fixture is unchanged (loops 4, bridges 3, aborted 3, guard_failures 600) and its traceback-shape oracle passes. The recorder also writes the fbw_* and field_pos_* keys that were absent from baselines recorded before those counters existed. Assisted-by: Claude * Trace traceback escape marking in exception attribute fold Assisted-by: Claude * Trace fresh container allocations in FOR_ITER callees Admit replay-safe fresh tuple and list allocation helpers during nested callee tracing. Specialize len() for empty-list storage and add a cross-backend parity fixture for the admitted shape. Assisted-by: Claude * Admit tuple copies from exact lists during replay Assisted-by: Claude * Identify traceback walk bridge training The 603 guard failures comprise three 200-hit bridge thresholds and three one-off transition failures. The final bridge reconnects the traceback walk to its compiled inner-loop token, so no resume-semantics change is required. Assisted-by: Claude * bench: add a synthetic fixture for the subscript inline's index operand `Seq.__getitem__` reached as `p[len(EMPTY)]`, so the index arrives on the operand stack rather than from a constant or a local, with an `isinstance(index, slice)` branch in the body so the inline has a residual to abort on. Prints 276000 under cpython, pypy3 and pyre. The defect the shape covers — the FOR_ITER deferred admission reading `arg_class_guard.is_none()` as a proxy for "the entry opcode is a CALL", which admitted the BINARY_OP-entered subscript inline and let the flush resume one operand short — is fixed in #1082, which names the property directly and carries its own parity test. This holds the shape under the jit-stats gate too. Assisted-by: Claude * bench: re-record the wasm pickle terminal-raise baseline `loops_compiled` 66 -> 67, `loops_aborted` 14 -> 15 and `guard_failures` 339 -> 356 on the wasm leg of `synth/pickle_terminal_raise_resume`. The file already carried the 356 from an earlier recording; the two loop counters did not. `retraces_compiled=0` joins the recorded set. Bisected to `jit: admit call-bearing LIST_APPEND bodies in the FOR_ITER gate` by in-place whole-tree control arms at four points of this branch: the base and the trees at the three commits below it read 66 / 14 / 339, the tree at that commit reads 67 / 15 / 356, and every counter moves there together. A base control arm reproduces main's committed 66 / 14 / 339 on this host, so the move is this branch's and not the host's. The dynasm and cranelift baselines for the same fixture are byte-identical to main's (30 compiled, 1 aborted, 338 guard failures) and are unchanged here: the loop the widened gate admits is one only the guest reaches, which compiles 66 loops in this fixture where the native backends compile 30. The extra abort is one more attempt at a loop the gate now allows, recorded beside the compile it gained. Not the collection schedule: `PYPY_GC_MIN` at 256MB, 384MB and 512MB gives identical counters, and three repeats agree exactly. Assisted-by: Claude * jit: pair the vable static shadow write with a heap write-back `mirror_vable_static_to_boxes` wrote `virtualizable_boxes` without the `synchronize_virtualizable()` half `_opimpl_setfield_vable` performs (pyjitpl.py:1188-1199). `walker_capture_snapshot_for_last_guard_impl` publishes `last_instr = py_pc - 1` through it, and the walk never runs the interpreter's own `frame.last_instr = pc` store, so the live frame stayed one opcode behind the shadow and `check_synchronized_virtualizable` (pyjitpl.py:3463-3468) failed under `debug_assertions` in `gc_stress::module_dict_move_to_end_reentrant_survives_python_callbacks`. Add `TraceCtx::synchronize_virtualizable_static`, a single-static `write_boxes` that keeps `synchronize_virtualizable`'s guards and its `VableArrayStorage::RustVec` carve-out. The full `write_all_boxes` is not usable here: the shadow's array half holds NULL for the operand slots a mid-opcode guard resumes before, and writing it back would stamp those NULLs into the live frame. Call it from `mirror_vable_static_to_boxes`. `try_execute_residual_call_via_executor` saves the `last_instr` shadow entry before publishing the executing pc and restores it after the residual returns, matching `LiveLastInstrGuard`'s save/restore of the heap half. The restore is skipped when the callee forced the virtualizable. Assisted-by: Claude * docs: list the two FOR_ITER gate diagnostics in gate-triage `PYRE_FOR_ITER_GATE_DIAG` (pyre-jit-trace/src/jitcode_dispatch/mod.rs, pyre-jit/src/eval.rs) and `PYRE_FORITER_INFLIGHT_CENSUS` (pyre-jit-trace/src/jitcode_dispatch/mod.rs) are read through `env::var_os(..).is_some()`, so both are default-OFF diagnostics and belong in §6c. `pyre/pyrex/tests/gate_triage_complete.rs ::every_live_pyre_gate_has_a_gate_triage_entry` failed on their absence. Assisted-by: Claude * bench: re-record ten jit-stats baselines after the rebase Rebasing onto `a7eb493079f` moved these counters. Measured on the final tree with all three backends rebuilt from a full `extract-llbc.py`. synth/pypy_type_surface (all 3) bridges_compiled 102 -> 5, guard_failures 20497 -> 1011 synth/mapdict_frozen_unboxing_fold (all 3) guard_failures 8 -> 11 synth/ca_bridge_multiframe_resume_double_call (wasm) guard_failures 2581 -> 2592 synth/closure_per_call (wasm) guard_failures 418 -> 426 synth/wasm_ca_trampoline_decline (wasm) guard_failures 404 -> 601 synth/recursion_memo_branch (wasm) guard_failures 4724 -> 4704 `pypy_type_surface` returns to the values #999 committed. #1086 had rewritten the file to 102 / 20497 — the numbers the `Cls.__name__` metaclass fold produced while it guarded the raw `w_class` slot its oracle's `gettypefor` fallback never read — and #1086 landed before #1106 declined that fold, so the file has named a defect since. The fixed fold gives 5 / 1011 again. `pypy_type_surface`, `ca_bridge_multiframe_resume_double_call` and `wasm_ca_trampoline_decline` are also red on main's own CI at `b925c3e5ead` (run 31283765874, ubuntu leg), so those three do not originate here. An in-place control arm reverting only this branch's vable shadow write-back reproduces every one of these numbers, so none of them is that change. Assisted-by: Claude * jit: follow the InflightForiterBody field rename in the census `InflightForiterBody::Jit` carries `jitcode_index: i32` since #1111, which also made the identity negative when unresolvable. The census `code_ptr` resolution still destructured the former `outer_jitcode_index: u32` and cast it, so the crate stopped compiling once both sides met. `raw_code_for_jitcode_index` indexes with the value, so a negative index misses and the census keeps the live frame's code. Assisted-by: Claude * mapdict: split the layout predicate from the storage predicate `has_mapdict_layout` answers the physical question — the allocation carries the `MapdictStorageMixin` slots — and no longer consults `w_type_get_hasdict` for the generated int/str/tuple user layouts. `has_mapdict_storage` is that test plus the owning class's `hasdict` flag, and `mapdict_carrier` now asserts the layout predicate, so a `__slots__`-only native subclass no longer trips the assertion. `is_generated_user_layout_family` carries the specialised-tuple exclusion for the layout test, the storage test, and the carrier's `W_TupleObjectUser` arm. Assisted-by: Claude * _structseq: re-read the pinned class after the tuple allocation The array-backed tuple constructor can collect, so the class pointer read before it can be stale when it is stored into the new object's `w_class`. Re-read it from the shadow-stack slot after the allocation. Assisted-by: Claude * jit: gate the in-flight FOR_ITER census key lookup on the census `raw_code_for_jitcode_index` runs `ensure_finish_setup` and borrows `METAINTERP_SD`; `fbw_foriter_inflight_take` called it on every take even though `census_record_foriter_inflight` returns immediately unless `PYRE_FORITER_INFLIGHT_CENSUS` or `PYRE_FBW_DEBUG_ABORT` is set. The enable check moves into `foriter_inflight_census_enabled`, which both sites share. Assisted-by: Claude * jit: correct the exception descr group note on w_context `w_context` is written by the raise lowering, not left zeroed by GC pointer clearing. Assisted-by: Claude * test: scan the loop-region fixture in two passes `loop_region_includes_out_of_line_handler_rejoining_mid_body` compared each backward target against `outer_header` while still lowering it, so a jump seen before the smallest target was missed. The scan now runs twice over a shared target closure, each pass with its own `OpArgState`. Assisted-by: Claude * test: cover synchronize_virtualizable_static Five cases: the single-field write-back, absent virtualizable state, an out-of-range index, a RustVec-backed array field, and a shadow slot holding no concrete. Assisted-by: Claude * majit: exclude the identity slot from the static write-back bound `virtualizable_values`'s last slot holds the vable identity (`virtualizable_boxes[-1]`), not a field value. `synchronize_virtualizable_static` bounded `index` by the full vector length, so a shadow shorter than the declared static count would have written the identity ref into a static field. Bound by the data length. Assisted-by: Claude * jit: read PYRE_FOR_ITER_GATE_DIAG through one accessor The per-opcode decline and the whole-region decline each owned a function-local `OnceLock` for the same variable. Assisted-by: Claude * jit: admit builtin subclass carriers in the mapdict storage helpers The seven mapdict residual wrappers tested their receiver with `is_instance`, which is true only for an ordinary `W_ObjectObject`. The generated int/str/tuple user layouts failed that test, and the wrappers answer a value rather than declining: the unboxed reads returned 0 and 0.0, the boxed read returned PY_NULL, and all three writes returned without storing. `Flag.__or__` reads `other._value_`, so `Perm.R & Perm.R` computed `4 & 0`; `test.test_enum`'s `OldTestIntFlag` test_and/test_or/test_xor/ test_type failed on that. Measured on the release dynasm build, an unboxed int attribute read on an int/str/tuple subclass was wrong 1756/ 2411/2498 times per run and correct under PYRE_NO_JIT=1. The receiver test is now `has_mapdict_layout`, which is `mapdict_carrier`'s own precondition, shared through `is_mapdict_carrier`. The parity fixture gains loops that validate the loaded and stored values for the unboxed int and float slots; the existing ones discard what they load and so never observed this. Assisted-by: Claude * Revert "jit: admit call-bearing LIST_APPEND bodies in the FOR_ITER gate" This reverts commit 2a0f91f. The decline it removed is load-bearing. A comprehension whose body calls a user Python function drops an element: `[random.randrange(25) for i in range(size)]` returned 22 items for size=23, and PYRE_FORITER_INFLIGHT_CENSUS reported DELIVERED=0 REFUSED=1 for that body pc on the same run. `test.test_heapq`'s test_heapsort failed on the shortened list, raising IndexError from `heappop`. The reverted commit argued the append always sits past the resume coordinate; the census shows the refusal path is reachable, because the call commits body effects that `fbw_foriter_inflight_take` sees as a committed effect since the consume. The parity fixture records the shape. Assisted-by: Claude * majit: drop the narrowed virtualizable static synchronizer `mirror_vable_static_to_boxes` now calls `synchronize_virtualizable()`, the shape `_opimpl_setfield_vable` uses (`pyjitpl.py:1188-1199`), so the narrowed single-field variant and its tests have no caller. Assisted-by: Claude * bench: restore five jit-stats baselines the reverted gate had moved The call-bearing LIST_APPEND admission raised loops_compiled and bridges_compiled on exception_group_type, list_append_virtual_payload, minmax_key_rooting, range_ctor_in_loop and subscr_user_getitem_stack_index; reverting it returns them to what main records. mapdict_frozen_unboxing_fold's guard_failures returns to 2, the value main carries — the branch's 11 was recorded while the mapdict storage helpers answered zero. dynasm only; the cranelift and wasm baselines follow. Assisted-by: Claude * bench: restore the cranelift jit-stats baselines to match Same six benches as the dynasm pass, same direction and magnitude. Assisted-by: Claude * bench: restore the wasm jit-stats baselines to match The same six benches as the dynasm and cranelift passes, plus global_store_plain_dict_globals and pickle_terminal_raise_resume, whose observed loops_compiled / loops_aborted / guard_failures all return to the values main records. closure_per_call keeps main's guard_failures: its loops_compiled and bridges_compiled are unchanged, so the count drifts without a shape change. 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.
Rebased onto
main(#1089). Three commits, down from nine — #1101 merged anearlier head of this branch, so six of the commits this PR used to carry are
already in
main. Replaying them produced duplicate definitions rather thanconflicts, which is how they were identified; the two that remain are the
wrong-code fixes, plus one doc commit whose code also landed in #1101.
No
.jitstatsbaseline is touched. 7 files, 255/69.jit: relocate a bridge's target tokens, and refuse a cross-buffer JUMP to an unrelocated targetassemble_bridgenever ran thecompiled_target_tokensrelocation thatassemble_loopinlined, so a LABEL assembled inside a bridge left itsll_loop_codeholding the in-buffer offset it was given at emission. A retraceis attached to a guard, so it is assembled through the bridge path and carries
its own LABEL; a later trace closing onto that token missed
target_tokens_currently_compiling, read the field, and baked the offset as anabsolute branch target —
br x16with 0x1fc, a deterministic SIGSEGV on anested loop whose inner accumulator changes type.
The relocation is extracted into
fixup_target_tokensand called from bothassemble paths, matching
aarch64/assembler.py:1087(called at:98and:213) andx86/assembler.py:990(:612,:706). Both dynasm backends hadthe omission; cranelift publishes an absolute address instead and is
unaffected. The list is consumed, so a token cannot be relocated twice.
A cross-buffer JUMP whose target is below the first page now fails the compile
rather than emitting the branch. Dynasm bakes the immediate at codegen, so
neither 0 (target never compiled) nor an unrelocated offset can be repaired
later; both are always wild branches.
jit_retrace_nested_loop_bridge_labelSIGSEGVs before this change.jit: name the FOR_ITER deferred-admit call-boundary property instead of inferring itThe
CalleeReplaySafety::DeferredCallarm admitted a callee whenarg_class_guard.is_none(), standing in for "the entry is a CALL the abortrewind can name". The arm's comment justified the proxy by asserting a binop
dunder dispatch was the only entry carrying an
arg_class_guard. It is not:try_walker_inline_subscr_getitementers fromBINARY_OPand passes noarg_class_guard, soobj[key]was admitted and resumed one operand short —the subscript index was replaced at runtime by an unrelated live Ref, failing
test.test_rein CI withtry_walker_inline_resolved_user_callnow takes an explicitentry_is_call_boundary. Every call site was audited against the oldpredicate's value;
try_walker_inline_subscr_getitemis the only behaviourchange (
true→false).What the gate asks is the entry opcode's stack effect, not the spelling: an
entry that only peeks re-executes from the stack the rewind already has.
opcode_for_iterpeeks its single iterator operand whereopcode_binary_oppops both of its own, and the FOR_ITER call site carries that reason rather
than a reference to the property routes. The two attribute entries reach their
stack discipline through
load_attr_cached/store_attr_cached, which thisaudit did not follow, so they keep the standing they had and say so.
jit_subscr_getitem_inline_keeps_its_indexraises that TypeError before thischange and is JIT-only.
jit: name which descr_getattribute arm each type-attribute fold coversThe folds landed in #1101; what remains is the cross-reference. The two folds
cover
typeobject.py:814-819(the metatype-data-descriptor arm__name__resolves through) and
:820-822(the class-MRO-value arm), and each doccomment now names its own arm and points at the other.
Verification
The full gate was run green on this content, but at an earlier SHA:
790ec11add24a3ab98b49ad98cd907410392da6eb9e3SNAPDIFF0,jit-stats regressed0; the last two backends ran at load 6-10,so they are not load-faked. Since then the branch was replayed onto a base that
moved by two commits / 2657 lines (#1110, #1089), touching
jitcode_dispatch/specialize.rsandmod.rsamong others, and onecomment-only amend was added. The three commits replayed without conflict and
the net diff is unchanged apart from those comments, but the numbers above
describe the older SHA — CI on this head is what gates it.
— authored by Claude
Summary by CodeRabbit
Bug Fixes
Tests