JIT walker: multi-pred trampolines + Int-bank concrete shadow - #89
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR refines interpreter exception canonicalization, adds an Int-bank concrete shadow and propagation throughout the JIT trace walker (with per-frame fresh register/concrete allocation), integrates the change into the shadow walker, emits per-link trampolines for Phase 4 multi-predecessor targets, and exposes/extends the majit executor with execute_pure_call plus tests. ChangesException handling refinement
JIT trace Int concrete value tracking
Phase 4 renaming and trampoline synthesis
Majit metainterp executor exposure and pure-call helper
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
youknowone
marked this pull request as ready for review
May 21, 2026 18:57
![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: 4c2f052bd3
ℹ️ 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".
Comment on lines
+1430
to
+1434
| if let super::flatten::Insn::Op { args, .. } = insn { | ||
| for arg in args.iter_mut() { | ||
| if let super::flatten::Operand::TLabel(tl) = arg { | ||
| if tl.name == from { | ||
| tl.name = to.to_string(); |
There was a problem hiding this comment.
This reverse scan rewrites the first matching TLabel in the whole block, not specifically the source branch terminator. In the multi-predecessor trampoline path, a fallthrough link can append goto <target> first, and then a later sibling link to the same target will rewrite that appended goto instead of the original goto_if_not/branch terminator. That misroutes edge-specific renamings (the explicit branch keeps pointing at the unrenamed target while the fallthrough path is retargeted), which changes control-flow semantics for blocks whose exits converge on one target.
Useful? React with 👍 / 👎.
Comment on lines
2037
to
+2038
| 'r' => { | ||
| // Recorded op result — walker doesn't compute the concrete | ||
| // (would need executing against the live heap), so pass | ||
| // Null. Downstream raise/r GUARD_CLASS treats Null as | ||
| // "no info, skip the guard". | ||
| write_ref_reg(ctx, op.pc, dst, result, ConcreteValue::Null)?; | ||
| write_ref_reg(ctx, op.pc, dst, result, concrete_for_shadow)?; |
There was a problem hiding this comment.
Here the ref-bank write uses concrete_from_recorded_opref unconditionally, which can produce ConcreteValue::Int/Float for boxed values. Storing those into concrete_registers_r breaks later ref-only consumers (getfield sanity loads / class-guard paths) that require ConcreteValue::Ref, so concrete propagation is lost and goto_if_not/iL can fail with ValueNotConcrete on real traces (the PopTop shadow-walker failure described in this patch is this exact shape). The ref shadow should only carry pointer concretes (or Null) at this write site.
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: 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/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 1529-1552: The loops over argboxes_r, argboxes_i, and argboxes_f
currently truncate oversized argument arrays by breaking when i >=
top_num_regs_*, producing a partially initialized frame; instead, detect when
argboxes_r.len() > top_num_regs_r (and likewise for argboxes_i/top_num_regs_i
and argboxes_f/top_num_regs_f) before the loops and return the same typed
arity/shape error used by the inline-call paths (or otherwise fail the bridge
fast) so callers get a clear arity mismatch rather than silent truncation;
update the bridge function in jitcode_dispatch.rs to perform these pre-loop
length checks and return the error, leaving the existing population logic for
top_regs_*/top_concrete_* and trace_ctx.box_value unchanged.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 1214-1317: Write two end-to-end tests that call
emit_trampoline_for_multi_pred_link and assert both emission arms: (1) for the
explicit-jump path (simulate rewrite_source_terminator_for_link returning
TerminatorRewrite::Rewritten) verify TrampolineOutcome::Emitted returns spam
containing a newly appended SpamBlockRef in all_walker_blocks, that the
synthetic block begins with the trampoline Label ("epsilon3_link_<n>"), contains
the renaming body_len insns and ends with a goto TLabel(target) plus
Unreachable, and that trampoline_counter incremented; (2) for the
fallthrough/default path (simulate TerminatorRewrite::FallthroughOrDefault)
verify TrampolineOutcome::Emitted returns spam equal to the original source
SpamBlockRef, that no new synthetic block was appended to all_walker_blocks, and
that the source_spam.per_block_ssarepr now has the renaming insns followed by
the explicit goto TLabel(target) and Unreachable (matching body_len). Use the
functions and types emit_trampoline_for_multi_pred_link,
rewrite_source_terminator_for_link, SpamBlockRef, TrampolineOutcome::Emitted,
and the TerminatorRewrite variants to locate and implement these checks.
🪄 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: 76a06f64-fc34-4c4e-8655-7ff9c420997e
📒 Files selected for processing (5)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/display.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/shadow_walker.rspyre/pyre-jit/src/jit/codewriter.rs
Comment on lines
+1214
to
+1317
| fn emit_trampoline_for_multi_pred_link<F>( | ||
| graph: &mut super::flow::FunctionGraph, | ||
| source_block: &super::flow::BlockRef, | ||
| link_ref: &super::flow::LinkRef, | ||
| get_color: &F, | ||
| all_walker_blocks: &[SpamBlockRef], | ||
| site: SpliceSite, | ||
| ) -> Option<SpliceShift> | ||
| all_walker_blocks: &mut Vec<SpamBlockRef>, | ||
| trampoline_counter: &mut usize, | ||
| ) -> TrampolineOutcome | ||
| where | ||
| F: Fn(&super::flow::Variable) -> u16, | ||
| { | ||
| let link_borrow = link_ref.borrow(); | ||
| let target_ref = link_borrow.target.clone()?; | ||
| let target_borrow = target_ref.borrow(); | ||
| if link_borrow.args.len() != target_borrow.inputargs.len() { | ||
| return None; | ||
| let pairs = collect_distinct_renaming_pairs(link_ref, get_color); | ||
| let body = build_renaming_insns(pairs); | ||
| if body.is_empty() { | ||
| return TrampolineOutcome::NoPairs; | ||
| } | ||
| let target_block = match link_ref.borrow().target.clone() { | ||
| Some(t) => t, | ||
| None => return TrampolineOutcome::NoPairs, | ||
| }; | ||
|
|
||
| // Locate the source SpamBlock for the in-place terminator rewrite. | ||
| let Some(source_spam) = all_walker_blocks | ||
| .iter() | ||
| .find(|s| !s.dead() && s.block() == *source_block) | ||
| .cloned() | ||
| else { | ||
| return TrampolineOutcome::RewriteFailed; | ||
| }; | ||
| let target_label = super::flatten::block_label_name(&target_block); | ||
| let trampoline_name = format!("epsilon3_link_{}", *trampoline_counter); | ||
| match rewrite_source_terminator_for_link( | ||
| &source_spam, | ||
| link_ref, | ||
| &target_label, | ||
| &trampoline_name, | ||
| ) { | ||
| TerminatorRewrite::Rewritten => { | ||
| *trampoline_counter += 1; | ||
| // Explicit-jump arm: the source's terminator (`goto_if_not`, | ||
| // `goto_if_not_int_is_zero`, `switch`, ...) carried this | ||
| // link's branch target. Synthesize a new SpamBlock for the | ||
| // trampoline so the rewritten terminator lands at | ||
| // `Label(<trampoline>)`, runs the ref_copies, then jumps to | ||
| // the original target. The block has no graph reachability, | ||
| // so the post-walk DFS reorder leaves it in append-order at | ||
| // the tail of `all_walker_blocks`. | ||
| let synthetic_block = graph.new_block(Vec::new()); | ||
| let trampoline_spam = SpamBlockRef::new(synthetic_block, None); | ||
| trampoline_spam.push_insn(super::flatten::Insn::Label(super::flatten::Label::new( | ||
| trampoline_name, | ||
| ))); | ||
| let body_len = body.len(); | ||
| for insn in body { | ||
| trampoline_spam.push_insn(insn); | ||
| } | ||
| trampoline_spam.push_insn(super::flatten::Insn::op( | ||
| "goto", | ||
| vec![super::flatten::Operand::TLabel( | ||
| super::flatten::TLabel::new(target_label), | ||
| )], | ||
| )); | ||
| trampoline_spam.push_insn(super::flatten::Insn::Unreachable); | ||
| all_walker_blocks.push(trampoline_spam.clone()); | ||
| return TrampolineOutcome::Emitted { | ||
| spam: trampoline_spam, | ||
| body_len, | ||
| }; | ||
| } | ||
| TerminatorRewrite::FallthroughOrDefault => {} | ||
| TerminatorRewrite::Missing => return TrampolineOutcome::RewriteFailed, | ||
| } | ||
|
|
||
| // Fall-through arm fallback: when no terminator TLabel matched the | ||
| // target's block label, the link is the fall-through arm of a | ||
| // multi-exit source (`flatten.py:264 make_link(linktrue)` runs | ||
| // immediately after `goto_if_not` and inlines the renamings before | ||
| // the target block body). Pyre's walker has no per-link `Label` | ||
| // for the fall-through arm — execution drops past the terminator | ||
| // straight into the next SpamBlock at byte-stream level. Append | ||
| // the renamings AFTER the source's terminator together with an | ||
| // explicit `goto TLabel(<target>)` + `---` tail so the target is | ||
| // reached deterministically regardless of post-walk DFS reorder. | ||
| // `strip_walker_block_boundary_goto` elides the explicit goto when | ||
| // the immediate next non-empty block opens with the target label. | ||
| let body_len = body.len(); | ||
| let mut spam_borrow = source_spam.0.borrow_mut(); | ||
| let insns = &mut spam_borrow.per_block_ssarepr; | ||
| for insn in body { | ||
| insns.push(insn); | ||
| } | ||
| insns.push(super::flatten::Insn::op( | ||
| "goto", | ||
| vec![super::flatten::Operand::TLabel( | ||
| super::flatten::TLabel::new(target_label), | ||
| )], | ||
| )); | ||
| insns.push(super::flatten::Insn::Unreachable); | ||
| drop(spam_borrow); | ||
| TrampolineOutcome::Emitted { | ||
| spam: source_spam, | ||
| body_len, | ||
| } | ||
| // Skip blocks whose target is `returnblock`/`exceptblock` | ||
| // (`is_final && exits.is_empty()`). Walker's RETURN_VALUE / | ||
| // RAISE handlers already emit `ref_return` / `raise` INLINE | ||
| // at the source block referencing the source stack-slot | ||
| // color (walker NEW-DEVIATION: upstream defers the return op | ||
| // to `make_return(target.inputargs)` AFTER insert_renamings | ||
| // copies link.args → target.inputargs). Splicing a | ||
| // `ref_copy stack_color → target_inputarg_color` BEFORE | ||
| // walker's existing `ref_return stack_color` leaves walker's | ||
| // terminator reading the un-copied source slot and breaks | ||
| // SSA allocator coalescing on trivial return functions. | ||
| if target_borrow.is_final && target_borrow.exits.is_empty() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add one end-to-end test for each trampoline emission arm.
The new tests only lock in rewrite_source_terminator_for_link. They still don't assert that emit_trampoline_for_multi_pred_link (a) appends a synthetic SpamBlock on the explicit-jump path or (b) appends renamings to the source block on the fallthrough/default path, so a regression in actual block emission/order would still pass.
🤖 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/src/jit/codewriter.rs` around lines 1214 - 1317, Write two
end-to-end tests that call emit_trampoline_for_multi_pred_link and assert both
emission arms: (1) for the explicit-jump path (simulate
rewrite_source_terminator_for_link returning TerminatorRewrite::Rewritten)
verify TrampolineOutcome::Emitted returns spam containing a newly appended
SpamBlockRef in all_walker_blocks, that the synthetic block begins with the
trampoline Label ("epsilon3_link_<n>"), contains the renaming body_len insns and
ends with a goto TLabel(target) plus Unreachable, and that trampoline_counter
incremented; (2) for the fallthrough/default path (simulate
TerminatorRewrite::FallthroughOrDefault) verify TrampolineOutcome::Emitted
returns spam equal to the original source SpamBlockRef, that no new synthetic
block was appended to all_walker_blocks, and that the
source_spam.per_block_ssarepr now has the renaming insns followed by the
explicit goto TLabel(target) and Unreachable (matching body_len). Use the
functions and types emit_trampoline_for_multi_pred_link,
rewrite_source_terminator_for_link, SpamBlockRef, TrampolineOutcome::Emitted,
and the TerminatorRewrite variants to locate and implement these checks.
walker_post_walk_insert_renamings silently elides multi-predecessor target renamings. Adds a count_distinct_renaming_pairs helper and an env-gated summary line per-graph reporting elided_links, elided_with_distinct_pairs, and elided_distinct_pairs_total. Sweep across 14 production + 25 synthetic benches: 6 graphs surface distinct-pair elisions (fannkuch x6 compiles, raise_catch_loop x1, synth bool_compare/exceptions/float_arithmetic/string_ops x1 each); the remaining 33 graphs have zero elisions because CFG coalesce in graph regalloc already unioned every src/dst pair.
collect_distinct_renaming_pairs returns the actual (src_color, dst_color, kind) triples and the probe now prints one record per elided link with src_pc / dst_pc plus the pair list. Detail across 6 affected graphs: - fannkuch: src_pc=-1 dst_pc=239 pairs=[ref:14->8] (x6 compiles) - raise_catch_loop: src_pc=46 dst_pc=-1 pairs=[ref:2->0] - synth/bool_compare: src_pc=16 dst_pc=47 pairs=[ref:4->2,ref:4->3] - synth/exceptions: src_pc=34 dst_pc=-1 pairs=[ref:3->1] - synth/float_arithmetic: src_pc=18 dst_pc=67 pairs=[ref:3->1,ref:3->2] - synth/string_ops: src_pc=20 dst_pc=113 pairs=[ref:5->3,ref:5->4]
…nes for multi-pred targets Replace the multi-predecessor skip in walker_post_walk_insert_renamings with a two-path trampoline synthesis. Explicit-jump arm (the source's terminator TLabel matches the target's block label) gets a synthetic SpamBlock appended to all_walker_blocks holding Label(<trampoline>) ; <ref_copy/push/pop ops> ; goto TLabel(<original target>) ; --- and the source's terminator TLabel is rewritten in place to point at the trampoline. Fall-through arm (no matching TLabel) appends the renamings plus an explicit goto+--- tail directly to the source's per_block_ssarepr; strip_walker_block_boundary_goto elides the goto when the immediate next non-empty block opens with the target label. walker_post_walk_insert_renamings now takes &mut FunctionGraph and &mut Vec<SpamBlockRef> so it can allocate synthetic flow::Blocks and append trampoline SpamBlocks. PYRE_PHASE4_DIAGNOSE_ELIDE=1 retains the residual-elide diagnostic and adds a phase4-trampoline counter line per graph; the previously known six elision cases (fannkuch x6, raise_catch_loop, synth/exceptions, synth/bool_compare, synth/float_arithmetic, synth/string_ops) now all report trampolines_emitted=1 and zero residual elide. check.py 39/39 dynasm + 39/39 cranelift PASS, 235 pyre-jit lib tests green.
…st-trampoline Drop the now-stale 'Known incompleteness — multi-predecessor targets' paragraph and the inline elision skip narration, replacing them with the unique-pred / multi-pred branch description matching the ε.3 trampoline + fall-through-append paths.
Verify the in-place rename of Operand::TLabel in the source SpamBlock's last Insn::Op matches the explicit-jump arm of emit_trampoline_for_multi_pred_link. Covers both the rename-success case (target name matches a goto_if_not's TLabel) and the absent-name case (rewrite reports false without mutating the block).
Extract build_renaming_insns from emit_link_renamings_into_block and have both it and emit_trampoline_for_multi_pred_link call into collect_distinct_renaming_pairs. Both callers now share the same last_exception / last_exc_value skip, src_color != dst_color check, returnblock guard, per-kind grouping, and reorder_renaming_list lowering. Refresh the in_degree precompute comment block to reflect the post-ε.3 dispatch (unique-pred TargetAfterAnchor vs multi-pred trampoline).
Add an env-gated diagnostic immediately after strip_walker_block_boundary_goto that counts surviving goto+--- pairs in ssarepr.insns by target-name prefix (pc / block / epsilon3_link_ / catch_landing_ / link / other). Task #111 investigation. Sweep across 11 representative benches shows every surviving goto+--- targets block_<addr> (canonical-style) or epsilon3_link_<N> (ε.3 trampoline tail). No walker-only pc{N} or other prefix surfaces, confirming the strip pass elides all walker block-boundary gotos that DFS reorder leaves contiguous.
…bels Extends the reverse-scan helper used by emit_trampoline_for_multi_pred_link to also walk Operand::Descr(DescrOperand::SwitchDict(_))._labels per liveness.py:76-78. Closes the explicit gap documented in phase4-trampoline-2026-05-21 (SwitchDictDescr TLabel entries were not yet walked, so a multi-pred switch-link elision would silently fail to rewrite its source terminator and trampoline emission would fall through to the append path). Rc<DescrOperand> is unwrapped via Rc::make_mut so the rewrite operates on a fresh copy if the descr is shared across paths. Adds rewrite_source_terminator_tlabel_rewrites_switch_dict_descr_label unit test covering a 3-entry SwitchDictDescr, asserts the matching label is rewritten and unrelated entries are untouched. cargo test -p pyre-jit --features dynasm --lib: 237 passed. pyre/check.py: dynasm 39/39 + cranelift 39/39 PASS.
…ed docs post-SwitchDictDescr walk Updates the doc comment on the multi-exit splice block and on the TrampolineOutcome::RewriteFailed variant to reflect that rewrite_source_terminator_tlabel now walks SwitchDictDescr._labels. RewriteFailed now refers to the explicit-jump-fallthrough split rather than the SwitchDictDescr-not-walked case.
Adds a sibling slice for Int-bank concrete shadow alongside concrete_registers_r. Every callsite passes `&mut []` for now; field is populated by Task #75.B (seed wiring) and follow-up slices. No consumer reads it yet, so the field is structurally visible but semantically inert. Updates the field docstrings on both concrete_registers_r and the new concrete_registers_i to cross-reference, describe the color-indexed invariant for the Int bank, and note the deferred seeding work. Mechanical plumbing: 99 callsites (test fixtures + 5 production sub-walks) gain the empty slice. cargo test -p pyre-jit-trace --features dynasm --lib: 228 PASS. cargo test -p pyre-jit --features dynasm --lib: 237 PASS. pyre/check.py: dynasm 39/39 + cranelift 39/39 PASS.
…walks + top-level entry allocate_callee_register_banks returns a 5-tuple including a fresh Vec<ConcreteValue> sized to total_i; callee constant Int slots seed ConcreteValue::Int(v) directly from body.constants_i. Three sub-walk dispatch helpers (inline_call_dr_kind + two siblings) destructure the new tuple and pass &mut callee_concrete_i into the sub-WalkContext. Top-level dispatch_via_miframe builds concrete_i_snapshot = vec![Null; sym.registers_i.len()] and passes &mut concrete_i_snapshot. PyreSym has no semantic-slot int seed (Python ints are boxed in concrete_locals on the Ref bank); Int-bank concretes are lazily populated by handler writes — Task #75.C onward. Test fixtures (~95 sites) keep &mut [] until they need Int concrete inputs. cargo test pyre-jit-trace 228 + pyre-jit 237 PASS. pyre/check.py 39/39 dynasm + 39/39 cranelift PASS.
…int_copy/i>i through write_int_reg Adds Int-bank twins of read_ref_reg_concrete + write_ref_reg from M4.Cutover Step 2.2. The helpers enforce the lock-step contract: every walker handler that writes registers_i[dst] must also update concrete_registers_i[dst] so downstream goto_if_not/iL and switch/id can fold the branch instead of surfacing GotoIfNotValueNotConcrete. int_copy/i>i becomes the first user — propagates the source slot's Int concrete shadow into the destination slot, paralleling the ref_copy/r>r Step 2.2 chain. Subsequent slices migrate int_<binop>, int_neg, getfield_gc_i, residual_call_*_i, etc. cargo test pyre-jit-trace 228 + pyre-jit 237 PASS. pyre/check.py: dynasm 39/39 + cranelift 39/39 PASS.
…oncrete fold When both int binop inputs have known concretes (via concrete_of_opref), compute the runtime result through try_fold_int_binop and stamp it onto the result OpRef via trace_ctx.set_opref_concrete. RPython parity: pyjitpl.py:execute_with_descr stamps box.value through the corresponding LLOp executor in rpython/jit/metainterp/executor.py. try_fold_int_binop covers the 14 always-pure ii>i opcodes pyre emits: IntAdd/Sub/Mul (wrapping), And/Or/Xor (bitwise), Lshift/Rshift (wrapping with shift count gated to 0..64 — out-of- range leaves the result without a concrete, matching RPython's int_lshift assumption that overflow goes through int_lshift_ovf), and IntLt/Le/Eq/Ne/Gt/Ge (returning 0 or 1 to mirror BoxInt). This unlocks goto_if_not/iL / switch/id to fold once both operands of a recorded binop are constants — without it, the result OpRef had no concrete and downstream surfaced GotoIfNotValueNotConcrete. binop_int_record also routes through the new write_int_reg helper so concrete_registers_i stays in lock-step with registers_i. cargo test pyre-jit-trace 228 + pyre-jit 237 PASS. pyre/check.py: dynasm 39/39 + cranelift 39/39 PASS.
… fold Extends the Task #75.D pattern from binop_int_record to the four int-unary opcodes pyre emits: IntNeg (wrapping), IntInvert (bitwise complement), IntIsZero, IntIsTrue (returning 0/1). Routes the write through write_int_reg so concrete_registers_i stays in lock-step. try_fold_int_unop mirrors RPython rpython/jit/metainterp/executor.py per-opcode LLOp executors. pyjitpl.py:execute_with_descr stamps box.value after running the executor; pyre matches via set_opref_concrete. cargo test pyre-jit-trace 228 + pyre-jit 237 PASS. pyre/check.py: dynasm 39/39 + cranelift 39/39 PASS.
…te_of_opref concrete_from_recorded_opref maps trace_ctx.concrete_of_opref(result) to a ConcreteValue (Int/Float/Ref/Null) for shadow write-back. The sentinel Value::Ref(GcRef(usize::MAX)) returned by concrete_of_opref on a miss maps to ConcreteValue::Null so the prior unknown-result contract holds. getarrayitem_gc_via_heapcache_with_index_bank, getfield_gc_via_heapcache, and getfield_vable_via_metainterp's 'i' and 'r' branches now route through write_int_reg / write_ref_reg with the derived concrete instead of an inline registers_i write + ConcreteValue::Null write_ref_reg. The 'f' branch stays inline (no concrete_registers_f shadow exists yet).
…e_int_reg/write_ref_reg The 'i' branch was inline-writing registers_i[dst] without touching concrete_registers_i; migrate to write_int_reg with concrete derived via concrete_from_recorded_opref. The 'r' branch was already on write_ref_reg but pinned to ConcreteValue::Null; switch to the same concrete derivation. CallPure* descrs whose recorded result lands a folded constant via concrete_of_opref now propagate.
Seven walker handlers wrote registers_i[dst] inline without touching the
concrete_registers_i shadow: binop_ref_to_int_record, ptr_nonzero_record,
binop_float_to_int_record, dispatch_inline_call_{dr,dir,dirf}_kind 'i' arms,
and cast_ptr_to_int/r>i. Migrate each to write_int_reg with a concrete
derived via concrete_from_recorded_opref so the Int-bank shadow stays
in lock-step with the OpRef writes. The three inline_call_* 'r' arms
already routed through write_ref_reg with ConcreteValue::Null also switch
to the unified shadow derivation, picking up constant SubReturn values
that surface through constants.get_value.
…ef concretes at entry Walker entry seeded only the slot-keyed concrete_registers_r/i banks; the OpRef-keyed opref_concrete table (TraceCtx::box_value chain) stayed empty. Iterate sym.registers_r and call set_opref_concrete for each non-constant OpRef whose corresponding concrete_r_snapshot slot is a Ref, mirroring Box.value population at trace-input record sites. shadow_walker.rs PopTop comment refreshed: the outer arm shape is still the 5-op contract the test fixture pins, but the inline_call_r_r chain recurses 4 levels into Rust helper bodies; getfield_gc_i on a small-int unboxed Ref-bank register still surfaces GotoIfNotValueNotConcrete on raise_catch_loop + synth/set_membership. Documented as Task #165. cargo test pyre-jit-trace 228 + pyre-jit 237 PASS; pyre/check.py dynasm 39/39 + cranelift 39/39 PASS; MAJIT_SHADOW_WALKER=1 baseline still has the 2 pre-existing PopTop failures pending Task #165 follow-up.
…eap pointer from concrete_vable_ptr Task #165.B follow-up: small-int unboxed locals (ConcreteValue::Int(n) in Ref-bank registers) hit getfield_gc_i with no Value::Ref concrete because the heap PyLong* was dropped at concrete_value_from_slot. Recover the actual PyObjectRef via state::concrete_stack_value (reading sym.concrete_vable_ptr's locals_cells_stack_w) and stamp opref_concrete from that source. Test-fixture path (concrete_vable_ptr=0) falls back to the slot-keyed Ref subset. Doesn't yet close raise_catch + synth/set_membership under MAJIT_SHADOW_WALKER=1: the failure chain extends to RefOp(57) created mid-walk, not just InputArgs at entry. Tracked in project_task165_poptop_shadow_int_concrete_2026_05_21.md. cargo test pyre-jit-trace 228 + pyre-jit 237 PASS; pyre/check.py dynasm 39/39 + cranelift 39/39 PASS.
…er file via PyPy MIFrame.__init__ + setup_call
…ot shadow PyPy parity (pyjitpl.py:executor.execute 'BoxPtr(value).value = result' / 'BoxInt(value).value = result'): at the SubReturn frame boundary, the callee's slot-keyed concrete_registers_<bank>[reg] holds the live value that the return slot carries up to the caller's writeback. Mirror it onto the OpRef-keyed opref_concrete table so the caller's concrete_from_recorded_opref(value) lookup in dispatch_inline_call_* returns the stamped Box.value instead of the Null sentinel. Skips constants (TraceCtx::constants.get_value is authoritative there) and Null / sentinel concretes (no useful value to mirror).
…ll argboxes have known concrete majit-metainterp gains pub fn executor::execute_pure_call(descr, func_ptr, args) which dispatches an elidable+cannot-raise residual_call directly through call_int_function / call_void_function without the MetaInterp/BH_LAST_EXC_VALUE seam that execute_varargs needs. Module visibility promoted from pub(crate) to pub. dispatch_residual_call_iRd_kind / iIRd_kind / iIRFd_kind invoke a new try_fold_pure_call_via_executor helper after record_op_with_descr. When the selector returns CallPure*, every allboxes entry resolves through TraceCtx::box_value, and the arity fits MAX_HOST_CALL_ARITY, the helper executes the callee at trace time and stamps the recorded OpRef via set_opref_concrete. RPython parity: pyjitpl.py:1346-1400 _record_helper_pure stamps result_box.value via executor.execute_varargs(opnum, argboxes, descr, exc=False, pure=True).
…GOTO probes Delete the two env-gated Phase 4 endgame diagnostic blocks plus elide-tracking variables (elided_links, elided_links_with_distinct_pairs, elided_distinct_pair_records, trampoline_records) and the last_op_summary_for_source helper that only served them. Convert TrampolineOutcome::RewriteFailed in walker_post_walk_insert_renamings to a fail-loud panic carrying graph name + target label + (src, dst, kind) pair list.
Six focused tests covering execute_pure_call: - single Int arg → Int result via call_int_function - three Int args → Int result (multi-arg ABI dispatch) - Float result via i64-bits packing through call_int_function - Void result via call_void_function returning the 0 sentinel - debug-asserts panic on EF_CANNOT_RAISE (non-elidable) - debug-asserts panic on EF_ELIDABLE_CAN_RAISE Casts function items through *const () before i64 to silence clippy's direct-function-cast warning.
…minator end Add `original_terminator_end: Option<usize>` to SpamBlock. The trampoline fallthrough fallback in `emit_trampoline_for_multi_pred_link` records the pre-append `per_block_ssarepr.len()` on its first append, then appends the synthetic `body + goto TLabel(target) + Unreachable` past that anchor. `rewrite_direct_terminator_tlabel` now caps its reverse scan at `original_terminator_end.unwrap_or(len)`. Sibling links whose explicit-jump rewrite shares the same target retarget the original branch terminator instead of the appended fallthrough goto.
write_ref_reg / write_int_reg collapse non-matching ConcreteValue kinds
(Int/Float in the ref shadow, Ref/Float in the int shadow) to Null. A
kind-mismatched stamp from concrete_from_recorded_opref no longer leaks
into the typed shadow and breaks downstream ref-only or int-only
consumers (getfield_gc_r sanity loads, goto_if_not/iL folds).
dispatch_via_miframe returns DispatchError::InlineCall{,Int,Float}
ArityMismatch when argboxes_*.len() > top_num_regs_* instead of
silently truncating via per-loop `break`. A caller/shape mismatch
surfaces as a typed failure rather than a partially seeded frame.
youknowone
changed the title
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3c4ac3b39
ℹ️ 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".
Comment on lines
+1513
to
1517
| let w_object = if pyre_object::is_bytes(w_object_in) { | ||
| w_object_in | ||
| } else { | ||
| let data = pyre_object::bytes_like_data(w_object_in); | ||
| pyre_object::w_bytes_from_bytes(data) |
There was a problem hiding this comment.
exc_unicode_decode_error_init now sends every non-exact-bytes object through bytes_like_data, but isinstance_bytes_like_w accepts bytes subclasses and bytes_like_data only supports exact bytes/bytearray layouts. For a bytes subclass input, this branch can reinterpret the object as a bytearray and read the wrong memory layout, which can crash or corrupt state while constructing UnicodeDecodeError. The old code only used bytes_like_data for actual bytearray objects, so this unsafe cast path is newly introduced.
Useful? React with 👍 / 👎.
Pure rustfmt output for the rebased commits' edits — no logic change.
…tearray layout exc_unicode_decode_error_init's non-exact-bytes branch fell through to bytes_like_data, which dispatches via py_type_check pointer identity and silently reads any non-exact-bytes object through the W_BytearrayObject layout. For a bytes subclass (e.g. `class MyBytes(bytes): pass`), the underlying struct is W_BytesObject, so the cast read the wrong memory layout. Split the post-filter branch with isinstance_w(obj, bytes) (subclass- aware): bytes subclass → w_bytes_data; bytearray (exact or subclass) → w_bytearray_data.
This was referenced May 24, 2026
Merged
This was referenced Jun 1, 2026
This was referenced Jun 24, 2026
This was referenced Jul 19, 2026
youknowone
added a commit
that referenced
this pull request
Jul 21, 2026
…tra_virtual_roots build break (#708) * comments: remove internal task/slice/gap tracking tags Session-invented tracking labels prefixed comments across 50 files: `task#50 phase-1`, `task #157`, `#73 S3.5`, `gh#73 S3.2`, `Parity #14 Slice C.4`, `Slice 7b`, `Slice C`, `sub-slice 4`, `micro-slice 3`, `gap-10`, `#203 gap-7`, `51d.1`, `M4`, `Phase G slice 2`. Each occurrence is replaced by the technical statement it prefixed; where the tag carried the sentence's subject the sentence is reworded. Dropped in the same lines: two claude memory-file paths cited as references (`item3_abstractstringrepr_epic_plan.md`, `project_issue73_architecture_walker_as_tracer_2026_05_28`), and the line numbers on file citations those comments carried. The `residual_call.rs` "Priority order for sub-slice 2 (widen)" paragraph is removed rather than reworded: it ordered work the same doc records as landed. Comment-only — no changed line is code. `cargo check --workspace --all-targets --features dynasm` passes; the pre-existing `vstack_mirror.rs` unreachable_patterns is the only warning. Assisted-by: Claude * majit-metainterp: initialize Snapshot::extra_virtual_roots at the guard-op multi-frame capture `capture_snapshot_for_last_guard_op_multi_frame_with_vable_vref` built a `recorder::Snapshot` without the `extra_virtual_roots` field, so majit-metainterp failed to compile with E0063 at `origin/main` `57b01d0e8bd`. The field was added by #661; this call site was not updated. Value and comment copied from the sibling `capture_snapshot_for_last_guard_multi_frame_with_vable_vref`, the same multi-frame path: the nested-list append fold resumes through the single-frame collapse, so no extra virtual roots reach here. Assisted-by: Claude * comments: remove AI-review labels, session plan/memory references, and remaining stage tags Second pass over the categories the first sweep did not cover. - `Codex P1` / `Codex P2` (+ `(PR #89)`, `(round 7/8/10)`) AI-review priority labels, 21 sites. The sentence each prefixed is kept. - References to files that exist only in a private session directory: `.claude/plans/*.md` (5), `memory/*.md` (6), and one `project_issue73_...` memory name. - Plan-stage identifiers: `S2.1`/`S2.2`/`S2.3`/`S2.4` "(wiggly-barto plan)", `S1.3`, `S0 spike`, `S1-S3`, `orth-9 step 4`, `Path B (B.6.7*)`, `[SPIKE-S0/FR]`, `A0-era`, `off-GC storage epic S2..S5`, `Task #85`/`#197`/`#333`, `#73 Slice-1`. Three changed lines are not comments, all string literals: the `S1.3` mentions in the `AnnotatorError` text and in an `expect_err` message in `rlib/jit.rs` / `extregistry.rs`, and a `memory/*.md` citation inside an `#[ignore = "..."]` reason string in `pyre-jit/src/eval.rs`. Every other changed line is a comment. `Finding #1`/`#2`/`#3` and `Option C` are kept: they are defined by the `#57` GitHub issue cited alongside them, so a reader can resolve them. `cargo check --workspace --all-targets --features dynasm` passes. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
The gate is read at majit/majit-ir/src/reg_write_audit.rs:131 and had no entry in this file, so `every_live_gate_has_a_triage_entry` in pyre/pyrex/tests/gate_triage_complete.rs failed on it (5 passed / 1 failed). With the row: 6 passed / 0 failed, all six tests read by name. The row is DERIVED FROM `reg_write_audit.rs`'s OWN DOC HEADER, not from reading the implementation and not from recall: the writer-attribution purpose, the ten production sites across two files, the `#[track_caller]` note-carries-its-own-call-site property, the off-path cost, and the thread-local rationale are all condensed from that header, which the row says. Two fields are read off the code because the header does not state them: the read site, and the default polarity — `resolve()` is `Ok(v) if v != "0" && !v.is_empty()`, so unset, empty and `=0` are off. The introduced-and-untouched-since marker is WITHHELD AS UNINFORMATIVE rather than omitted by oversight, and the row says which: `git log -S` over the name returns exactly one commit, but that commit is hours older than this row, so "never revisited" is entailed by the gate's age rather than being a fact about the gate. Retirement condition is UNRECORDED — the module's doc header states none, so there is no sentence to quote. THIS ROW DOES NOT RESOLVE A ROW-COUNT DISAGREEMENT, BECAUSE NO ROW COUNT IS ASSERTED ANYWHERE. Task #89's record dates the catalog at 51 rows; the file carries 52 gate rows before this commit and 53 after. Neither `every_live_gate_has_a_triage_entry` nor `every_live_triage_entry_still_has_a_reader` asserts a count — both assert SET EQUALITY between the gates read from the environment and the gates documented live in this file. A reader comparing 51 against 53 is comparing a dated figure with a current one, not reading a check that moved. The file is 0 of 729 closure inputs on the 4-crate argv and 0 of 733 on the 6-crate one, with scripts/llbc_extract.py and majit/majit-metainterp/src/lib.rs reading IN at both apertures, so this stales nothing. 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.
Fix #27
Summary
Self-review
Prompt & Model
Model:
Prompt:
Answer
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests