Repository navigation
jitcode_dispatch: route bank-crossing casts via macro table - #680
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughThe JIT trace walker now uses shared comparison, guard, and cast recorders with concrete-value propagation, adds integer and float concrete shadows, centralizes guarded branches, expands opcode routing and tests, removes extra snapshot roots from one path, and refreshes parity documentation. ChangesJIT trace dispatch and recording
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 9d9e4c4). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26beaed87f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let condbox = record_int_cmp(ctx, opcode, a, b); | ||
| let switchcase = match ctx.trace_ctx.concrete_of_opref(condbox) { |
There was a problem hiding this comment.
When a fused goto_if_not_<cmp> compares the same non-float box (for example goto_if_not_int_eq r0,r0) or two constants, the branch result is statically known and the trait tracer's fused path folds it before recording. This path always records a new compare and then requires concrete_of_opref(condbox); if the operands are symbolic but identical it returns GotoIfNotValueNotConcrete, aborting a trace that should be followed without a guard, and constants get an unnecessary guard/snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b6bd7903d
ℹ️ 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 let (Some(majit_ir::Value::Ref(la)), Some(majit_ir::Value::Ref(rb))) = | ||
| (ctx.trace_ctx.box_value(a), ctx.trace_ctx.box_value(b)) |
There was a problem hiding this comment.
When goto_if_not_ptr_eq/rrL or _ptr_ne compares ref registers whose OpRefs are not stamped but whose runtime pointers are available in ctx.concrete_registers_r (the shadow channel used by other ref handlers such as ptr_nonzero), this helper records the compare but leaves its result without an Int concrete. fused_goto_if_not_ptr immediately calls concrete_of_opref(condbox) and returns GotoIfNotValueNotConcrete, so those branches abort instead of following the known runtime direction; pass the register concretes into this path and stamp from the shadow before branching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99aa8dec72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let both_constant = b1.is_constant() && b2.is_constant(); | ||
| let (resbox, overflow) = record_int_ovf(ctx, opcode, b1, b2); |
There was a problem hiding this comment.
When both operands are Const, PyPy's opimpl_int_*_jump_if_ovf returns a constant result or jumps to the overflow label without emitting an overflow op/guard pair. This records Int*Ovf first, and the later both_constant path branches without a guard, leaving an overflow op not immediately followed by GuardNoOverflow/GuardOverflow (a structural invariant checked by history::check_consistency and assumed by the backends' pending overflow flag). A *_jump_if_ovf with constant operands can therefore produce an invalid trace or let the stale overflow flag affect a later guard; fold the constant case before recording, or keep the guard paired with the ovf op.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e22fec9b15
ℹ️ 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".
| guard_opcode, | ||
| &[], | ||
| taken_pc, | ||
| other_pc, |
There was a problem hiding this comment.
When a trace is recorded through the overflow arm and a later iteration does not overflow, this passes other_pc as op.next_pc into guarded_branch_core, whose snapshot resumes at the not-taken pc. Overflow guards are not like goto_if_not: PyPy's handle_possible_overflow_error records generate_guard(..., resumepc=orgpc) (rpython/jit/metainterp/pyjitpl.py:1881-1889) so the blackhole re-executes the int_*_jump_if_ovf opcode and writes the >i result on the no-overflow path. Resuming directly at fallthrough skips the arithmetic and leaves the destination register as NONE/stale for subsequent ops; capture overflow guard resumes at op.pc/orgpc instead of the branch target.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/arith.rs`:
- Around line 86-99: Update record_int_ovf to return a typed DispatchError when
either concrete_of_opref lookup does not yield a Value::Int, replacing both
operand unreachable! panics. Ensure int_ovf_jump checks and propagates this
error from record_int_ovf so the driver can fall back rather than crashing;
preserve the existing overflow computation for valid integer operands.
🪄 Autofix (Beta)
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
Run ID: 75500ed1-59d9-4643-a244-201aa7808eda
📒 Files selected for processing (13)
pyre/pyre-jit-trace/src/jitcode_dispatch/arith.rspyre/pyre-jit-trace/src/jitcode_dispatch/branch.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/heapcache_ops.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rspyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62306e2d6a
ℹ️ 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".
| // index outstrips a signed byte; it still occupies exactly one | ||
| // operand byte (a patched const-pool index instead of the literal), | ||
| // and the body below only skips that byte, so both forms share it. | ||
| "jit_merge_point/cIRFIRF" | "jit_merge_point/iIRFIRF" => { |
There was a problem hiding this comment.
When the assembler emits jit_merge_point/iIRFIRF for a jdindex outside the signed-byte range, this new arm shares the c-form body, but the body later still computes jdindex from the raw code[op.pc + 1] byte. For the i form that byte is the patched const-pool/register slot, not the actual driver index, so the loop-header assertion and jitdrivers_sd.get(jdindex) use the wrong driver or fail for indices >127; resolve the int operand through the register/constant path before sharing the rest of the body.
Useful? React with 👍 / 👎.
| // a concrete pointer in hand. The walker often has no shadow for a | ||
| // ref slot, and an absent shadow is not evidence of non-nullness, | ||
| // so decline rather than feed the check a value it did not observe. | ||
| let ConcreteValue::Ref(ptr) = read_ref_reg_concrete(code, op, 0, ctx) else { |
There was a problem hiding this comment.
When assert_not_none/r is applied to a ref whose non-nullity is already known in the heapcache but whose concrete shadow is absent (for example a new/d>r or new_with_vtable/d>r result: those handlers call new_object but write ConcreteValue::Null), this early shadow check returns UnsupportedOpname instead of allowing TraceCtx::trace_assert_not_none to take its known == Some(true) fast path. That makes valid traces abort immediately after these newly handled allocation paths; call the heapcache-aware helper before demanding a concrete pointer, or only demand concrete when nullity is not known.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ffef90e5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let ConcreteValue::Ref(ptr) = concrete else { | ||
| return Ok((DispatchOutcome::Continue, op.next_pc)); | ||
| let concrete = match bank { | ||
| GuardValueBank::Int => read_int_reg_concrete(code, op, 0, ctx), |
There was a problem hiding this comment.
When a promoted Int value has been routed through the Int scratch renaming path (int_push/i/int_pop/>i), the destination register can still hold an OpRef with a concrete box_value, but the Int concrete shadow is not propagated there (unlike the Ref scratch path). Reading only concrete_registers_i here makes int_guard_value/i fall into the no-op arm, so the required GUARD_VALUE for a promoted green can be omitted and the trace can be compiled for one observed value without guarding it; fall back to the OpRef concrete value or carry the Int scratch concrete alongside the OpRef.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a3d07a0b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let ConcreteValue::Ref(ptr) = read_ref_reg_concrete(code, op, 0, ctx) else { | ||
| return Err(DispatchError::UnsupportedOpname { | ||
| pc: op.pc, | ||
| key: op.key, | ||
| }); |
There was a problem hiding this comment.
When this pointer branch is fed by a ref whose nullity is already known in the heap cache but whose concrete ref shadow is intentionally absent—for example the new new/d>r and new_with_vtable/d>r paths call new_object but write ConcreteValue::Null—this early shadow read returns UnsupportedOpname before the heapcache fast path below can run. In this walker the heapcache is the only available trace-time proof for those freshly recorded allocations, so valid traces through if new_obj: / if not new_obj: abort instead of following the known non-null arm; check is_nullity_known/box_value before demanding read_ref_reg_concrete.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
Fold cast_int_to_float / cast_int_to_ptr / cast_ptr_to_int into a single unop_cast_record helper in arith.rs and add them to regular_record_table!. These were the last opnames from the pyjitpl.py:356-368 exec-generated unary loop still hand-spelled as handle() arms (one bespoke helper plus two inline blocks); the typed register banks make each cast a distinct src/dst-bank+fold shape, so the helper selects on the opcode. handle() loses the three cast arms. The record macro table is now the complete trace-side mirror of the exec-generated opimpl set pyre emits; every remaining hand-written family maps to a PyPy hand-written def or alias. Both backends 225/225. Assisted-by: Claude
Factor the branch core of the goto_if_not/iL arm into
goto_if_not_branch_on, then add FBW-tracer arms for the 14 fused
goto_if_not_<cmp> opcodes (int/float compares iiL/ffL, ptr eq/ne rrL).
record_{int,float,ptr}_cmp record the compare and stamp the concrete
truth without a register write; fused_goto_if_not_{int,float,ptr} read
the two operands, record the compare, resolve the switch from its
concrete, and delegate to the shared branch core. These opcodes are
not emitted until the rtyper cutover, so the arms exercise the same
guard/resume/kept-stack path the unfused arm already runs.
Assisted-by: Claude
pyjitpl.py:1513-1515 aliases opimpl_{int,ref,float}_guard_value to one
_opimpl_guard_value, but the FBW tracer handled only ref_guard_value/r and
declined int_guard_value/float_guard_value to the interpreter. Generalize
ref_guard_value_record into a bank-parameterized guard_value_record
(GuardValueBank Int/Ref/Float) and add the two missing arms; the ref path is
unchanged. Add read_float_reg_concrete, the float twin of read_int_reg_concrete.
Correct verified-stale comments: guard_value_record and dispatch_switch_id
claimed their guards record empty resume data, but both attach a production
snapshot via walker_capture_snapshot_for_last_guard; note that
record_{int,ptr,float}_cmp do not pre-fold the all-constants / b1-is-b2 case
(pyjitpl.py:2648-2661), consistent with the sibling binop_*_record recorders.
t3_audit walker-handled opnames 136 -> 138.
Assisted-by: Claude
Strip `file.ext:NNN` line-number suffixes from prose comments across the jitcode_dispatch tracer, keeping the filename and symbol name. The symbol is a stable anchor; the line number drifts whenever the upstream file shifts (e.g. handle_possible_exception was cited at 2156-2168, now at 3404; finishframe_exception at 2506, now 2530). The curated `//!` module doc-table is left intact. Three citations named the correct symbol but pointed at another function's line range (handle_possible_exception at _do_jit_force_virtual's lines, execute_raised at opimpl_raise's); dropping the line number removes the mismatch. Also refine the getarrayitem_gc_*_pure comment: the ConstPtr+ConstInt constant-fold is skipped because both operands are non-constant OpRefs on the walker, not because it is "structurally unreachable". Comment-only; no behavior change. Assisted-by: Claude
Handle the overflow-jump opcodes in the FBW tracer, the last of
pyjitpl's exec-generated opimpl loops. record_int_ovf records the
INT_{ADD,SUB,MUL}_OVF op and sets its concrete result from checked
arithmetic. int_ovf_jump mirrors opimpl_int_add_jump_if_ovf and
handle_possible_overflow_error: a non-constant overflow emits an
operand-less GUARD_OVERFLOW and jumps to the handler label; no overflow
emits GUARD_NO_OVERFLOW and writes the result to dst; a both-constant
operand pair branches without a guard.
The branch core of goto_if_not_branch_on is extracted into
guarded_branch_core (guard opcode/operands, taken/resume pc, optional
result write) so the overflow arm reuses the same kept-stack, resume
snapshot, and trampoline path. goto_if_not_branch_on stays
behaviour-preserving.
Unit tests cover the overflow-jump and no-overflow-continue cases.
Assisted-by: Claude
Handle `new_with_vtable/d>r` in the FBW tracer. The arm mirrors opimpl_new_with_vtable, which delegates to execute_new_with_vtable: record NEW_WITH_VTABLE carrying the size descr, then mark the result in the heapcache with new_object (heapcache.new) and class_now_known. pyre's class_now_known takes the vtable address, read off the descr's SizeDescr, where upstream only raises the known-class flag. Operand layout `d>r` is a 2-byte descr index followed by the 1-byte ref destination, matching the emit order in the jitcode assembler. The result carries no recording-time concrete: the walk never allocated a real object, so readers decline instead of consuming a stale value — the posture new_array_clear keeps when it cannot materialize a backing block. t3_audit walker-handled 140 -> 141. Every opname with JitCode hits is now either handled or, for the `unreachable` abort marker, declined by design. Assisted-by: Claude
…ed ops
`binop_int_record`, `unop_float_record`, and `binop_ref_to_int_record`
already carried the folds for these opcodes -- `eval_binop_i` covers the
six unsigned members with u64 semantics, `eval_unary_f` covers FloatAbs,
and `binop_ref_to_int_record` has InstancePtrEq/InstancePtrNe arms -- but
the opname rows were missing from `regular_record_table!`, so the walker
declined jitcodes carrying them.
Upstream lists all nine inside the same exec-generated `opimpl_*` loops
the table mirrors: `uint_rshift`/`uint_mul_high`/`uint_{lt,le,gt,ge}` in
the binop loop, `float_abs` beside `float_neg` in the unary loop, and
`instance_ptr_{eq,ne}` beside `ptr_{eq,ne}` in the compare loop. The
assembler can emit each key (`record_binop_i`, `record_unary_f`,
`record_binop_r`), and the legacy dispatcher already traces the unsigned
ops from `BC_UINT_*`.
Extract `drive_float_unop` from the inline `float_neg` test to match the
neighbouring `drive_int_unop` idiom, and cover the nine new rows.
Walker-handled opnames 141 -> 150.
Assisted-by: Claude
`new/d>r` mirrors the neighbouring `new_with_vtable` arm minus the class stamp: upstream `opimpl_new` delegates to `execute_new`, which records NEW against the size descr and seeds `heapcache.new`, and a plain struct carries no vtable word so nothing is known about its class. The two heapcache hints delegate to `TraceCtx::trace_assert_not_none` and `TraceCtx::trace_record_exact_class`, the same bodies the legacy dispatcher drives from `BC_ASSERT_NOT_NONE` / `BC_RECORD_EXACT_CLASS`, so the two tracers share one implementation of the cache gate, the record, and the follow-up stamps. `trace_assert_not_none` asserts a non-null concrete because it mirrors `do_assert_not_none`, which fatalerrors on null. The walker frequently holds no shadow for a ref slot, and an absent shadow is not evidence of non-nullness, so that case declines rather than feeding the check an unobserved value. Also move the `new_array_clear` comment block back onto its own arm -- adding `new_with_vtable` had left it stranded above the wrong handler -- and extract `drive_alloc_with_descr` plus `run_hint_step` for the tests. Walker-handled opnames 151 -> 154. Assisted-by: Claude
The unary member of the fused-goto family. Upstream `opimpl_goto_if_not_int_is_zero` records the condition and hands it to `opimpl_goto_if_not` with `replace=False`, so the handler mirrors `fused_goto_if_not_int` with one operand read and reuses `goto_if_not_branch_on` unchanged. `record_int_unary` is the one-operand counterpart of `record_int_cmp`, folding through `eval_unary_i`. Operand offsets match the sibling `goto_if_not/iL` arm, which carries the same argcodes: register at operand 0, label at operand 1. `goto_if_not_int_is_true/iL` gets no arm -- the assembler spells that condition as plain `goto_if_not/iL`, matching the class-attribute alias upstream gives it, so the opname is never emitted. Walker-handled opnames 154 -> 155. Assisted-by: Claude
The assembler switches from `jit_merge_point/cIRFIRF` to `jit_merge_point/iIRFIRF` once the jitdriver index no longer fits a signed byte, writing a patched const-pool index in place of the literal. Both spellings occupy exactly one operand byte and the handler only skips that byte before reading the six typed lists, so the existing body covers the wide form unchanged. Walker-handled opnames 155 -> 156. Assisted-by: Claude
Raw memory carries no heapcache bookkeeping. Where the `_gc_` family consults and updates the cache, upstream's raw pair only records: `opimpl_raw_load_i` is `execute_with_descr(rop.RAW_LOAD_I, arraydescr, addrbox, offsetbox)` and `_opimpl_raw_store` delegates to `execute_raw_store`, which is `execute_and_record(rop.RAW_STORE, ...)` and nothing else. Recording alone matches the walker's posture for stores: `setfield_gc_via_heapcache` records and updates the cache without performing the write, so the walk stays symbolic. The load result gets no recording-time concrete -- the walk never performed the access -- so readers decline rather than consume a value the walker did not observe. `run_hint_step` grows a descr-pool variant for the two tests. Walker-handled opnames 156 -> 158. Assisted-by: Claude
`goto_if_not_ptr_nonzero` / `goto_if_not_ptr_iszero` are not condition-plus- branch like the compare-fused gotos: upstream routes both through `_establish_nullity`, which emits the nullity guard itself and returns `box.nonnull()` as the branch direction. The two arms differ only in which way that answer sends them. `establish_nullity` follows the upstream body: short-circuit on a heapcache nullity hit (bumping HEAPCACHED_OPS), else guard NONNULL when the pointer is non-null and its class is not already known, or guard ISNULL and promote the box to the null constant, then stamp the nullity. Pyre's `is_nullity_known` splits upstream's boolean into which side is known, so upstream's "known at all" test maps to `is_some`. Guard emission stays walker-side because resume snapshots are: `walker_emit_guard_with_snapshot` is the walk's `generate_guard`. `box.nonnull()` needs the pointer, which the walker has only via the concrete shadow; an absent shadow is not evidence of either nullity, so that case declines rather than picking a branch direction. The shared test harness now carries a resolvable outer resume coordinate so guard-emitting arms can be driven from a unit test at all. Walker-handled opnames 158 -> 160. Assisted-by: Claude
Upstream `opimpl_hint_force_virtualizable(box)` is a thin forward to `gen_store_back_in_vable(box)`, and `TraceCtx::gen_store_back_in_vable` already hosts that whole body -- the `vinfo is None` gate, the nonstandard-virtualizable and already-forced early returns, and the SETFIELD_GC / SETARRAYITEM_GC / token-clear flush. The arm reads its one ref operand and hands it over, matching the legacy dispatcher's `opimpl_hint_force_virtualizable`. Walker-handled opnames 160 -> 161. Assisted-by: Claude
#690 retired the trace-time single-executor forks and removed `WalkContext::is_full_body_walk`; the four test fixtures added on this branch still set it. Assisted-by: Claude
…e arms - `int_ovf_jump` resumes its overflow guard at the opcode's own pc, matching `handle_possible_overflow_error`'s `resumepc=orgpc`, so the blackhole re-executes the arithmetic and writes the `>i` destination. It previously passed the not-taken branch pc, the `goto_if_not` convention. - `record_int_ovf` folds constant operands to a `const_int` without recording the `Int*Ovf` op, matching the `isinstance(resbox, Const)` arm that emits neither op nor guard; the branch-without-guard case no longer leaves an overflow op unpaired with its guard. - `record_int_ovf` returns `IntOvfOperandNotConcrete` instead of `unreachable!()` when an operand carries no concrete integer. - `jit_merge_point/iIRFIRF` resolves its jdindex through the int bank; the shared body reads that byte as the driver index, so the `i` spelling was reading a register slot. - `assert_not_none/r` consults the heapcache before demanding a ref shadow, so a `new/d>r` or `new_with_vtable/d>r` result whose nullity is already known no longer declines. - `record_int_cmp` folds the same-box and all-constant cases (`FASTPATHS_SAME_BOXES`), and `record_ptr_cmp` stamps its result from the ref shadows, so fused compares follow the known direction instead of surfacing `GotoIfNotValueNotConcrete`. Assisted-by: Claude
…ush/int_pop `int_pop/>i` wrote `registers_i[dst]` directly, leaving `concrete_registers_i[dst]` holding the shadow of the value the move overwrote. Every later `read_int_reg_concrete` on that register then reported the overwritten value, so `int_guard_value/i` could pin the wrong constant or skip its GUARD_VALUE, and `goto_if_not/iL` / `switch/id` could fold on it. Stash the source shadow in `WalkSession::tmpreg_i_concrete` and pop through `write_int_reg`, mirroring `ref_push/r` / `ref_pop/>r`. The Float bank resolves concretes from the OpRef rather than a side table, so `float_pop/>f` needs no companion field; say so where it reads. Assisted-by: Claude
…pare arms `opimpl_%s(b1, b2): if b1 is b2: return <const>` covers ten opnames: `int_eq`, `int_ne`, `int_lt`, `int_le`, `int_gt`, `int_ge`, `ptr_eq`, `ptr_ne`, `instance_ptr_eq`, `instance_ptr_ne`. Only the fused `goto_if_not_<cmp>` helpers implemented it, so the macro-table arms recorded a compare for an identical operand pair, and `instance_ptr_eq` / `instance_ptr_ne` had no fast path on any path. Lift `FASTPATHS_SAME_BOXES` into `fastpath_same_boxes` and consult it from `binop_int_record` and `binop_ref_to_int_record` as well. Membership decides, not opcode shape: `uint_lt` / `uint_le` / `uint_gt` / `uint_ge` are compares that belong to the other generated loop and must still record. Assisted-by: Claude
…resume-pipe removal Removing the extra_virtual_roots channel deleted the field from `recorder::Snapshot` and its initializers, but missed the multi-frame vable/vref site in `capture_snapshot_for_last_guard_op_multi_frame_with_vable_vref`, leaving the sole reference to the deleted field. majit-metainterp did not compile under either backend. Drop it as the sibling sites were dropped. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4891fb3504
ℹ️ 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".
| OpCode::PtrNe => (la != rb) as i64, | ||
| _ => panic!("record_ptr_cmp: unsupported opcode {opcode:?}"), | ||
| }) | ||
| } else if let (ConcreteValue::Ref(la), ConcreteValue::Ref(rb)) = (a_concrete, b_concrete) { |
There was a problem hiding this comment.
When a freshly recorded allocation is compared with CONST_NULL by the new fused goto_if_not_ptr_eq/rrL or _ptr_ne path, the allocation only has heapcache non-nullity because the new allocation arms write a ConcreteValue::Null ref shadow. Both the box_value check above and this concrete-shadow fallback then fail, leaving the compare unstamped; fused_goto_if_not_ptr immediately requires a concrete Int and aborts even though the branch direction is known. Fold comparisons against null from heapcache nullity before leaving the compare symbolic.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d9e4c48f8
ℹ️ 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".
| // The walk never performed the load, so the result carries no | ||
| // recording-time concrete and readers decline instead of | ||
| // consuming a value the walker did not observe. | ||
| write_int_reg(ctx, op.pc, dst, result, ConcreteValue::Null)?; |
There was a problem hiding this comment.
When a traced raw load feeds goto_if_not/iL or switch/id, this writes ConcreteValue::Null, so concrete_of_opref is unset and the very next branch returns GotoIfNotValueNotConcrete. The existing metainterp path for the same opcode reads base + offset and stores the concrete with set_int_reg (majit/majit-metainterp/src/pyjitpl/dispatch.rs:2924-2951); the walker needs to do the same instead of accepting raw loads that immediately decline.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
| ctx.trace_ctx | ||
| .record_op_with_descr(OpCode::RawStore, &[base, offset, value], descr); |
There was a problem hiding this comment.
When a successful full-body walk reaches raw_store_i, this only records IR; it never applies the store to the live raw address. The metainterp's path reads concrete base/offset/value and writes descriptor-sized memory while tracing (majit/majit-metainterp/src/pyjitpl/dispatch.rs:2862-2897), so a traced raw-storage write in the recording iteration is skipped until a later compiled iteration or entirely if the trace exits, leaving raw buffers stale.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
Summary
Continues the
jitcode_dispatchgeneration-parity work landed in #671.pyjitpl.py:356-368's exec-generated unary loop includes the castscast_int_to_float/cast_ptr_to_int/cast_int_to_ptr. These werethe last opnames from that generated loop still hand-spelled as
handle()arms (one bespoke helper + two inline blocks). This folds them into a
single
unop_cast_record(code, op, ctx, opcode)inarith.rsand addsthem to
regular_record_table!. pyre's typed register banks make eachcast a distinct (src-bank, dst-bank, concrete-fold) triple, so the helper
selects the shape on the opcode; PyPy can generate them from one template
only because its boxes are untyped.
handle()loses the three cast arms. The record macro table is now thecomplete trace-side mirror of the exec-generated opimpl set pyre emits;
every remaining hand-written family maps to a PyPy hand-written def or
alias (
opimpl_getfield_gc_i/r/f,_opimpl_setfield_gc_anyaliases,_opimpl_residual_call1/2/3,bhimpl_*_copy/push/return, …) and staysexplicit for parity.
Verification
cargo check -p pyre-jit-trace --features dynasm— cleanregular_record_table_is_the_sole_dispatcher+t3_audit_opname_gap_inventory— greencheck.py --backend dynasm— 225/225check.py --backend cranelift— 225/225Follow-up
The one remaining generation-parity item is the fused
goto_if_not_*family (
pyjitpl.py:541), which is codewriter-level (pyre emits anunfused
goto_if_not/iL+ separatecompare_op). That work will stackon this branch.
🤖 Generated with Claude Code
Summary by CodeRabbit