jit: delete the retired MIFrame value-op layer, trait, and parent_frames (Phase-6 S4+S5) by youknowone · Pull Request #686 · pyre-lang/pyre

youknowone · 2026-07-20T09:47:50Z

Phase-6 — delete the retired MIFrame trait-driven meta-interpreter (S4 + S5)

The FBW walker (jitcode_dispatch) is the sole production tracer; it never
dispatches opcodes through the MIFrame trait impls, and it builds its own
multi-frame resume snapshot through PendingInlineFrame / InlineParentFrame.
This branch deletes the now-dead trait-leg. MIFrame the type stays
production-live — only its trait impls, the orphaned value-op layer, and the
always-empty parent-frame chain are removed.

Commits:

S4-core — delete impl TraceHelperAccess for MIFrame, the inherent
MIFrame value-op methods, and the 16+1 MIFrame-typed generated_*(&mut MIFrame)
fns in typed_trace.rs. Keep the WalkerFrameOps-generic
generated_*<F: WalkerFrameOps> (production) plus the shared helpers.
S4-comments — retire dangling doc-comment references to the deleted symbols.
S4-trait — delete the now-implementorless TraceHelperAccess trait and
the 22 emit_trace_* free fns whose only callers were its default bodies.
Keep the 9 walker-called emit fns and
emit_trace_call_{int,ref}_typed_elidable_cannot_raise (exercised by
elidable_helper_canary_test.rs).
S5 — delete the vestigial MIFrame.parent_frames multi-frame chain: the
field, its 12 Vec::new() initializers, the four dead helpers
(extend_types_with_parents, build_framestack_frames,
materialize_parent_frame_state, materialize_parent_snapshot_state), and
the two always-dead guard branches (generate_guard's !is_empty() multi-frame
snapshot path and concrete_valuestackdepth's !is_empty() early return). The
single-frame build_framestack_snapshot path is unchanged; the live walker
PendingInlineFrame / InlineParentFrame path is untouched.
store_token orphan — delete the dead MIFrame::store_token_in_vable
(no callers; the walker emits the force-token setfield + GUARD_NOT_FORCED_2
through fbw_store_token_in_vable).

Rebased onto origin/main (#646, the WalkSym walker genericization). An earlier
GC-root stopgap commit on this branch was dropped: #684 (and the #646 merge)
landed the canonical immortal register_pyre_class_offsets registration for the
_collections deque iterators (and _tokenize iterator) on main, so the
branch carries no GC-registration change.

Verification

cargo check -p pyre-jit-trace -p pyre-jit -p pyre-interpreter --all-targets --features dynasm — 0 errors.
cargo test (CI --all --no-default-features --features dynasm) — green. A pre-existing off-by-one in jit: genericize the trace walker over WalkSym; unify interpreter/GC subclass ranges (#205 C2) #646's dormant jd1 unit test (unpack_state::tests::jd1_build_time_descrs_resolve_through_global_pool) is corrected here: the extracted _unpackiterable_unknown_length merge point sits at pc 0x5b, but the test and UnpackSym::loop_header_pc hard-coded 0x5c (this test was already red on all three OSes at jit: genericize the trace walker over WalkSym; unify interpreter/GC subclass ranges (#205 C2) #646's own merge). Unrelated to the MIFrame deletion — this branch touches no census/decode/LLBC machinery; the correction just re-syncs the hard-coded constant to the fresh-extraction value.
python3 pyre/check.py — dynasm 241/241 (local macOS/aarch64 dynasm+cranelift, and CI ubuntu dynasm). The CI ubuntu-x86_64 run shows three pre-existing, non-regression failures also present on main/jit: genericize the trace walker over WalkSym; unify interpreter/GC subclass ranges (#205 C2) #646 and outside this diff's scope (this branch touches no wasm/cranelift/synth/perf files, and dynasm is green in the same run — proving the shared trace side): cranelift int_loop (perf-threshold flake, 0.30s vs 0.14s on a loaded runner) and two wasm-backend "wrong output" cases (synth/delete_negative_open_slice_hot, synth/selfrec_tail_exception_unwind), which are known wasm-Linux-x86_64 gaps.

🤖 Generated with Claude Code

coderabbitai · 2026-07-20T09:47:58Z

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 11a4228e-b51c-4409-b5dd-ce4547e0304f

📥 Commits

Reviewing files that changed from the base of the PR and between b159a2f and 41a1aff.

📒 Files selected for processing (19)

majit/majit-ir/src/effectinfo.rs
majit/majit-metainterp/src/box_trace.rs
majit/majit-metainterp/src/trace_ctx.rs
pyre/pyre-jit-trace/src/helpers.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
pyre/pyre-jit-trace/src/state.rs
pyre/pyre-jit-trace/src/trace_helpers/typed_trace.rs
pyre/pyre-jit-trace/src/trace_opcode.rs
pyre/pyre-jit-trace/src/unpack_state.rs
pyre/pyre-jit-trace/src/walker_frame_ops.rs
pyre/pyre-jit/src/call_jit.rs
pyre/pyre-jit/src/eval.rs
pyre/pyre-jit/src/jit/codewriter.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch rewrite-tracer

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-20T10:01:43Z

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 41a1aff).
Updated: 2026-07-20T23:27:01.680Z

Files in the reviewed diff

majit/majit-ir/src/effectinfo.rs
majit/majit-metainterp/src/box_trace.rs
majit/majit-metainterp/src/trace_ctx.rs
pyre/pyre-jit-trace/src/helpers.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
pyre/pyre-jit-trace/src/state.rs
pyre/pyre-jit-trace/src/trace_helpers/typed_trace.rs
pyre/pyre-jit-trace/src/trace_opcode.rs
pyre/pyre-jit-trace/src/unpack_state.rs
pyre/pyre-jit-trace/src/walker_frame_ops.rs
pyre/pyre-jit/src/call_jit.rs
pyre/pyre-jit/src/eval.rs
pyre/pyre-jit/src/jit/codewriter.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

pyre/pyre-jit/src/eval.rs:4498 ↔ pypy/interpreter/baseobjspace.py:1015: JD1_LOOP_HEADER_PC remains 0x5c, although the extracted jit_merge_point corresponding to PyPy’s unpack-loop header is at 0x5b. upstream/main already contained this stale constant; the patch corrected only UnpackSym and its assertion. Runtime tracing still enters via this old constant.

4. Structural adaptations

pyre/pyre-jit-trace/src/unpack_state.rs:47 ↔ pypy/interpreter/baseobjspace.py:1015: Rust stores the generated JitCode byte offset 0x5b for PyPy’s source-level jit_merge_point; the source has no equivalent numeric PC. This is a necessary source-translation adaptation, and the patch’s 0x5b value matches the extracted-jitcode assertion.

…-6 S4-core) After S1-S3 retired the MIFrame trait-driven dispatch, this value-op cluster had no production callers (the FBW walker uses walker-native fast-paths). Delete: - impl TraceHelperAccess for MIFrame (trace_opcode.rs). - the inherent MIFrame value-op bridge methods (binary/compare/subscr, iter_next, unpack_sequence, build_tuple, direct_len/abs/type/isinstance/minmax, truth/unary, store_subscr, store_local/delete_local, trace_known_builtin_call, handle_reraise/handle_raise_varargs, seed_raised_exception, finishframe_exception, build_pending_inline_frame, inline_function_call, try_trace_exception_new, and the vable/vref/stack push/pop/peek/swap helpers). - the six dead capture_* facades (capture_record_branch_guard, capture_concrete_branch_truth_for_value, capture_trace_dynamic_list_index, capture_generated_list_getitem_by_strategy, capture_direct_len_value, capture_iter_next). - state.rs box_value_for_python_helper / box_args_for_python_helper. - the 16 MIFrame-typed generated_* fns in typed_trace.rs + opimpl_check_neg_index. The WalkerFrameOps-generic generated_* fns, the shared helpers (load_items_block, check_index_in_bounds, list_len_descr_for_strategy), and the production MIFrame methods (load_local_value, trace_guarded_int_payload, concrete_at, generate_guard, close_loop_args_at, ...) stay. Delete nine unit tests that exercised the removed methods. Retarget test_branch_guard_preserves_pre_pop_stack_shape and test_branch_truth_uses_concrete_parameter to their capture_generate_guard + snapshot assertions, dropping the removed capture_record_branch_guard / capture_concrete_branch_truth_for_value arms. The TraceHelperAccess trait and the emit_trace_* helpers are retired in a follow-up. cargo check --all-targets clean; cargo test green; check.py dynasm 225/225, cranelift 225/225. Assisted-by: Claude

…ayer (Phase-6 S4-comments) Update comments across the trace/jit tree (and two majit files) that named symbols deleted in S4-core (generated_* typed fns, trace_build_tuple_value, trace_plain_int_payload, compare_value_direct, binary_int_value, seed_raised_exception, build_pending_inline_frame, try_trace_exception_new, emit_call_assembler_callee_frame, box_value_for_python_helper, ...) so no comment references a removed symbol. Where a comment cited a deleted fn as the structural model for surviving walker-native code, keep the structural description and retire-tag or drop the symbol name. record_branch_guard / store_local_value references naming the surviving pyre-interpreter trait methods are left; only the trace_opcode / MIFrame references are updated. The majit-translate codegen path reference and the pyjitpl.py upstream references stay. Comment-only; cargo check green. Assisted-by: Claude

…ase-6 S4-trait) The `impl TraceHelperAccess for MIFrame` was removed in S4-core, leaving the `TraceHelperAccess` trait with no implementor. Its default-method bodies were the sole callers of a set of `emit_trace_*` free fns in helpers.rs. Delete: - the `pub trait TraceHelperAccess` definition (helpers.rs) - 22 `emit_trace_*` free fns whose only callers were the deleted trait bodies - the now-dead private fn `trace_name_args` and 11 private imports it used - the orphaned `use crate::helpers::TraceHelperAccess;` in the state.rs test module Kept the 9 emit fns with walker callers, and emit_trace_call_{int,ref}_typed_elidable_cannot_raise, which tests/elidable_helper_canary_test.rs exercises directly. Assisted-by: Claude

…ase-6 S5) MIFrame.parent_frames was populated only by the retired trait-interpret leg; the FBW walker builds its multi-frame resume snapshot through PendingInlineFrame / InlineParentFrame instead. The field was always empty on every production path, so its consumers were dead: - generate_guard's `if !self.parent_frames.is_empty()` multi-frame snapshot branch (never taken) - concrete_valuestackdepth's `if !self.parent_frames.is_empty()` early return (always false) - build_framestack_frames / extend_types_with_parents and the materialize_parent_frame_state / materialize_parent_snapshot_state helpers they called Remove the field, its 12 `Vec::new()` initializers, the four dead helpers, and the two dead guard branches. The single-frame snapshot path (build_framestack_snapshot) is unchanged. PendingInlineFrame's own `parent_frames` (the live walker inline-parent path) is untouched. Assisted-by: Claude

MIFrame::store_token_in_vable had no callers; the FBW walker emits the force-token setfield + GUARD_NOT_FORCED_2 through fbw_store_token_in_vable (jitcode_dispatch/fbw_state.rs) instead. The wrapped TraceCtx helper store_token_in_vable_setfield stays live (called by the FBW port). Assisted-by: Claude

The extracted `_unpackiterable_unknown_length` jitcode carries its `jit_merge_point` at pc 0x5b. Update the hard-coded value in `UnpackSym::loop_header_pc` and the `jd1_build_time_descrs_resolve_through_global_pool` assertion, which both read 0x5c. Assisted-by: Claude

`should_inline` / `should_inline_with_ctx` / `should_inline_core` have had no callers since the MIFrame value-op layer was retired in #686; the FBW walker runs the same gate independently. They stay because they are the only metainterp-side home of two pieces of `_opimpl_recursive_call` that the live `recursive_decision` closure does not carry: the `dont_trace_here` side effect it defers until its producer slice is wired, and the not-tracing arm. A sweep of every in-tree and out-of-workspace consumer of majit-metainterp (aheui-jit, cel-jit, the vendored aheui) finds no reference to any of the three. Assisted-by: Claude

…es (#1266) * jit: widen the walker CALL_ASSEMBLER fold past four pyre-local declines `try_walker_call_assembler_self_recursive` declined on four conditions that `_opimpl_recursive_call` / `do_residual_call` (`pyjitpl.py`) do not carry. Each is replaced by the narrower question it was standing in for: - Callee identity: a callee that is neither the walk's own code nor already on the framestack was refused outright. It is now admitted when its cell carries `JC_DONT_TRACE_HERE` (`can_inline_callable` false), the condition upstream puts on `assembler_call`, and its body carries no `raise`, no exception table and no `abort_permanent` marker — the same body facts the inline route screens on. - Caller exception table: the fold asked whether the caller's body held a `catch_exception` anywhere. It now asks whether the CALL's own coordinate falls inside a protected region, excluding the whole-body depth-zero `lasti` entry 3.14 wraps around every generator, per the discrimination `code_yields_inside_try` (`pycode.rs`) already makes. - Executed-body residual: the latch that declines a fold after a foreign non-pure residual ran now exempts the fold's own concrete-stamp executor by scope (`SELFREC_CA_FOLD_ACTIVE`) rather than by callee code identity, so a successful fold no longer latches the gate against later calls in the same walk. - Loop-carried operands: an InputArg below the call declined the fold regardless of what it held. A numeric accumulator no longer does; a live iterator still does, because folding under one costs the caller a compiled loop and a bridge on `recursive_call_frame_relocation`. Measured on dynasm, same tree, interleaved: a depth-8 recursion called a million times from a `while` body goes 1.21s -> 0.06s, depth-9 0.74s -> 0.06s; `recursive_call_frame_relocation`, `fib_recursive`, `generator_tree_recursion` and `foriter_call_resume_drops_iteration` are unchanged. `pyre/check.py` is 436/436 on both backends. Three synthetic jit-stats baselines are re-recorded for lower `guard_failures`: `ca_bridge_multiframe_resume_double_call` 3437 -> 3369, `recursion_memo_branch` 3554 -> 3491, `recursive_forced_frame_kept_stack` 800 -> 600. Assisted-by: Claude * bench/synth: add a recursion past the unroll bound driven from a loop body `step` recurses nine frames deep, two past FBW_MAX_INLINE_RECURSION, and is called from a `while` body that keeps a numeric accumulator on the value stack under the call. No existing fixture holds that pair: in `fib_recursive` and `selfrec_bridge_nontail_promote` the recursion is itself the hot loop, and `recursive_call_frame_relocation` puts a `FOR_ITER` iterator below the call instead of a numeric operand. Records `max-pypy-ratio=6` and the three backend jit-stats baselines (loops_compiled=2, bridges_compiled=4, guard_failures=651 on all three). Assisted-by: Claude * comments: correct three claims about upstream's recursive-call path `inline_call.rs` said `foreign_callee_admits_call_assembler` "reproduces the condition `_opimpl_recursive_call` puts on `assembler_call`". pyjitpl.py:1417 puts no condition there — once control reaches the fall-through it emits unconditionally. The screen is this fold's own precondition, which is what the function's own doc comment already says. The same correction applies to the message of 85d1621, which describes the foreign admission as requiring `JC_DONT_TRACE_HERE`; the gate reads body facts only and does not consult `can_inline_callable`. `decide_recursive_inline` and the `recursive_call` dispatcher both said "There is no `compile_tmp_callback` here". It is ported in `compile.rs` and two seams synthesise through it (`direct_assembler_call`, `get_or_make_portal_assembler_token_arc`). What is actually absent is a route from those seams to this decision: the predicate is pure in its five scalars, and the dispatcher reaches token resolution only through `Runtime::recursive_call_assembler_target`. Assisted-by: Claude * pyjitpl: record why should_inline is kept without callers `should_inline` / `should_inline_with_ctx` / `should_inline_core` have had no callers since the MIFrame value-op layer was retired in #686; the FBW walker runs the same gate independently. They stay because they are the only metainterp-side home of two pieces of `_opimpl_recursive_call` that the live `recursive_decision` closure does not carry: the `dont_trace_here` side effect it defers until its producer slice is wired, and the not-tracing arm. A sweep of every in-tree and out-of-workspace consumer of majit-metainterp (aheui-jit, cel-jit, the vendored aheui) finds no reference to any of the three. Assisted-by: Claude * jit: emit the top-level frame's traceback node into the trace A trace that finishes with `exit_frame_with_exception` recorded its own root frame's traceback node only for the recording pass; the compiled trace carried no runtime record, on the premise that the interpreter would record it when the trace handed back the error. That premise holds only for a trace the interpreter entered. A CALL_ASSEMBLER entry comes from another trace's compiled code, and the descr dispatches to `handle_fail_exit_frame_with_exception`, which republishes the exception and returns into the caller's machine code — no interpreter sees the error, so the entered frame contributed no node. The traceback was one frame short per CALL_ASSEMBLER entry, i.e. one per `FBW_MAX_INLINE_RECURSION + 1` levels of a recursion the walker folds: `rec(9)` ending in `1 // 0` gave 10 `rec` frames interpreted and 9 compiled. `fbw_publish_exit_last_instr` already publishes the raise coordinate the node needs, and the emitted recorder falls back to that same field. The interpreter-entry case stays at one node: `screen_frame_already_recorded` clears `attach_tb` when the chain head already names the frame. Assisted-by: Claude * bench/synth: re-record three improved wasm jit-stats baselines guard_failures fell on wasm the same way it did on the two native backends when the walker CALL_ASSEMBLER fold widened: ca_bridge_multiframe_resume_double_call 3437 -> 3369, recursion_memo_branch 3554 -> 3491, recursive_forced_frame_kept_stack 800 -> 678. The native baselines were recorded in 85d1621; these are the wasm halves. Assisted-by: Claude * jit: emit the exit-frame traceback node after the vable store-back The runtime half of the node was emitted beside the recording half, ahead of `fbw_publish_exit_last_instr` and `fbw_force_virtualizable_before_return`. It is the only consumer of the frame on that arm, so reading the frame there moved the escape ahead of the store-back: wasm synth/comprehension_object_append_hot went bridges_compiled 14 -> 17, guard_failures 2810 -> 3410. Emitting it after the store-back leaves the traceback unchanged — the publish has already settled `last_instr`, which the recorder falls back to — and returns that bench to its recorded baseline. Assisted-by: Claude

youknowone force-pushed the rewrite-tracer branch from 141459c to d224f03 Compare July 20, 2026 16:05

youknowone added 5 commits July 21, 2026 01:49

youknowone force-pushed the rewrite-tracer branch from d224f03 to d421d77 Compare July 20, 2026 17:35

youknowone changed the title ~~jit: delete the retired MIFrame value-op layer + TraceHelperAccess trait (Phase-6 S4)~~ jit: delete the retired MIFrame value-op layer, trait, and parent_frames (Phase-6 S4+S5) Jul 20, 2026

youknowone merged commit 9da9b5f into main Jul 21, 2026
29 of 31 checks passed

youknowone deleted the rewrite-tracer branch July 21, 2026 03:39

Navigation Menu

Repository navigation

jit: delete the retired MIFrame value-op layer, trait, and parent_frames (Phase-6 S4+S5) - #686

jit: delete the retired MIFrame value-op layer, trait, and parent_frames (Phase-6 S4+S5)#686
youknowone merged 6 commits into
mainfrom
rewrite-tracer

youknowone commented Jul 20, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading

Review limit reached

Uh oh!

github-actions Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Phase-6 — delete the retired MIFrame trait-driven meta-interpreter (S4 + S5)

Verification

Uh oh!

coderabbitai Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

github-actions Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Codex parity review

1. Regressions to PyPy parity introduced by this patch

2. Other mismatches introduced by this patch

3. Pre-existing mismatches (already present before this patch)

4. Structural adaptations

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Jul 20, 2026 •

edited

Loading

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading