Repository navigation
pyre-jit-trace: decompose jitcode_dispatch into per-cluster submodules - #666
Merged
Merged
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 (10)
WalkthroughAdds Rust JIT trace-walker support for arithmetic, heapcache and virtualizable operations, bridge sub-walks, branch resumption, guard snapshots, PC-map diagnostics, operand-stack mirroring, and dispatcher validation. ChangesJIT trace-walker execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 160-183: Update unop_int_record to handle OpCode::SameAsI
explicitly as an identity fold, preserving the operand’s concrete integer value
without calling eval_unary_i. Keep the existing eval_unary_i path for IntNeg,
IntInvert, IntIsTrue, and IntIsZero, and continue writing the resulting concrete
value through write_int_reg.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 1-193: Confirm that all eight benchmarks were run after the
JIT-source change and that none regressed; do not finalize the change if any
benchmark fails or regresses. Also run scripts/extract-llbc.py to re-extract the
affected Charon .ullbc files before rebuilding the rtyper prepass, and record or
verify these validation steps for the jitcode_dispatch module relocation.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs`:
- Around line 160-198: Introduce a shared WalkContext test fixture or builder
and replace the repeated WalkContext literals throughout the tests with it. Seed
the helper with the current common defaults, including sentinel vstack/live
fields, descriptor defaults, and fail descriptors, while allowing tests to
override fields such as registers, descriptor pools, is_top_level, and
is_authoritative_executor. Update all affected test setup sites to use the
helper so future WalkContext fields require changes in one place.
🪄 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: 985543f4-7163-4b36-afac-60a2958db609
📒 Files selected for processing (10)
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/diag.rspyre/pyre-jit-trace/src/jitcode_dispatch/heapcache_ops.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.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.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 160-183: Update unop_int_record to handle OpCode::SameAsI
explicitly as an identity fold, preserving the operand’s concrete integer value
without calling eval_unary_i. Keep the existing eval_unary_i path for IntNeg,
IntInvert, IntIsTrue, and IntIsZero, and continue writing the resulting concrete
value through write_int_reg.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 1-193: Confirm that all eight benchmarks were run after the
JIT-source change and that none regressed; do not finalize the change if any
benchmark fails or regresses. Also run scripts/extract-llbc.py to re-extract the
affected Charon .ullbc files before rebuilding the rtyper prepass, and record or
verify these validation steps for the jitcode_dispatch module relocation.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs`:
- Around line 160-198: Introduce a shared WalkContext test fixture or builder
and replace the repeated WalkContext literals throughout the tests with it. Seed
the helper with the current common defaults, including sentinel vstack/live
fields, descriptor defaults, and fail descriptors, while allowing tests to
override fields such as registers, descriptor pools, is_top_level, and
is_authoritative_executor. Update all affected test setup sites to use the
helper so future WalkContext fields require changes in one place.
🪄 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: 985543f4-7163-4b36-afac-60a2958db609
📒 Files selected for processing (10)
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/diag.rspyre/pyre-jit-trace/src/jitcode_dispatch/heapcache_ops.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.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
🛑 Comments failed to post (3)
pyre/pyre-jit-trace/src/jitcode_dispatch/arith.rs (1)
160-183: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm eval_unary_i does NOT handle SameAsI (would panic), and that int_same_as reaches unop_int_record. rg -nP --type=rust -C2 'fn eval_unary_i' echo '--- SameAsI arms in eval_unary_i ---' ast-grep run --pattern 'fn eval_unary_i($_, $_) -> $_ { $$$ }' --lang rust 2>/dev/null | rg -n 'SameAsI|other =>|panic!' echo '--- other unop_int_record fold call sites ---' rg -nP --type=rust -C1 'eval_unary_i\(' pyre/pyre-jit-trace/src/jitcode_dispatch/arith.rsRepository: youknowone/pyre
Length of output: 470
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- arith.rs around unop_int_record ---' sed -n '150,190p' pyre/pyre-jit-trace/src/jitcode_dispatch/arith.rs echo '--- eval_unary_i definition ---' rg -n -C 6 'fn eval_unary_i' majit -g '*.rs' echo '--- OpCode variants / SameAsI references ---' rg -n -C 3 'SameAsI|IntNeg|IntInvert|IntIsTrue|IntIsZero' majit pyre -g '*.rs' echo '--- regular_record_table mappings for int_same_as ---' rg -n -C 2 'int_same_as|SameAsI' pyre/pyre-jit-trace/src -g '*.rs'Repository: youknowone/pyre
Length of output: 50372
Handle
SameAsIseparately inunop_int_record.int_same_as/i>iis routed througheval_unary_i, but that helper only acceptsIntNeg/IntInvert/IntIsTrue/IntIsZeroand panics onSameAsI. A concrete source value will take this path and abort the tracer; foldSameAsIas identity before the generic unary evaluator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit-trace/src/jitcode_dispatch/arith.rs` around lines 160 - 183, Update unop_int_record to handle OpCode::SameAsI explicitly as an identity fold, preserving the operand’s concrete integer value without calling eval_unary_i. Keep the existing eval_unary_i path for IntNeg, IntInvert, IntIsTrue, and IntIsZero, and continue writing the resulting concrete value through write_int_reg.pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs (1)
1-193: 🧹 Nitpick | 🔵 Trivial
Confirm benchmark + Charon re-extraction steps for this JIT-source change.
As per coding guidelines, "After JIT changes, run all eight benchmarks and do not commit if any benchmark regresses" and "After changing interpreter, object, or JIT source, re-extract the corresponding Charon
.ullbcfiles withscripts/extract-llbc.pybefore rebuilding the rtyper prepass." The PR objectives mention formatting, Rust tests, and Python checks (dynasm/cranelift) but not the benchmark suite or Charon re-extraction. Since this is described as a pure relocation, these should be no-ops, but the guideline applies to any JIT-source change and is worth confirming was run given the file's scale and criticality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs` around lines 1 - 193, Confirm that all eight benchmarks were run after the JIT-source change and that none regressed; do not finalize the change if any benchmark fails or regresses. Also run scripts/extract-llbc.py to re-extract the affected Charon .ullbc files before rebuilding the rtyper prepass, and record or verify these validation steps for the jitcode_dispatch module relocation.Source: Coding guidelines
pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs (1)
160-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Consider a
WalkContexttest builder to collapse the repeated 40-field literal.This exact
WalkContext { ... }initializer (≈40 fields, most set to the same defaults —callee_shadow: None,raw_descrs: RawDescrPool::Global, all thevstack_*/live_*sentinels, the fourdone_with_this_frame_*fail descrs, etc.) is copy-pasted across ~55 tests. This dominates the file and makes it fragile: adding oneWalkContextfield forces edits at every call site, and a mis-set field in any one copy is easy to miss.A small helper (e.g. a
WalkContextFixture/builder seeded fromfresh_trace_ctx, or aDefault-backed constructor taking only the fields a test overrides — registers, descr pool,is_top_level,is_authoritative_executor) would let each test set only what it exercises and cut thousands of lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs` around lines 160 - 198, Introduce a shared WalkContext test fixture or builder and replace the repeated WalkContext literals throughout the tests with it. Seed the helper with the current common defaults, including sentinel vstack/live fields, descriptor defaults, and fail descriptors, while allowing tests to override fields such as registers, descriptor pools, is_top_level, and is_authoritative_executor. Update all affected test setup sites to use the helper so future WalkContext fields require changes in one place.
Rename jitcode_dispatch.rs to jitcode_dispatch/mod.rs and add the `// __SUBMODULES__` marker anchor so the module can be split into per-cluster submodules. No logic change. Assisted-by: Claude
Move the `#[cfg(test)] mod tests` block into tests.rs (declared from mod.rs). Its `include_str!` self-scan now targets mod.rs. No test logic change. Assisted-by: Claude
Move the jitcode-pc -> python-pc inversion helpers (python_pc_for_jitcode_pc + floor-boundary), skip_python_trivia_forward, resolve_parent_resume_py_pc, the in-flight FOR_ITER body resolvers, and the surviving RECIPE_RESULTCOLOR audit probe into diag.rs. Assisted-by: Claude
Move the int_*/float_*/ptr_* kind-parameterized record helpers (binop/unop recorders, pure-binop eval, ref/ptr guards, cast) into arith.rs. Assisted-by: Claude
Move the bridge sub-walk drivers into bridge_subwalk.rs, the heapcache-backed field/array recorders (plus the vable field-index constants) into heapcache_ops.rs, and the virtualizable field/array metainterp accessors into vable_ops.rs. Assisted-by: Claude
Move the value-stack mirror (classify/reconcile/reseed/seed/step/ enter-handler) and the pc-map block-head helpers it steps through into vstack_mirror.rs. Assisted-by: Claude
Move the branch-target resolution / trampoline decoding / branch-arm resume-liveness helpers into branch.rs, and the guard resume-data snapshot capture (single- and multi-frame inline paths plus the inline-caller-frame computation) into resume_snapshot.rs. Assisted-by: Claude
Replace the 25 uniform `HELPER(code, op, ctx, OpCode::VARIANT)` dispatch arms for the int_*/float_*/ptr_* record families with a `regular_record_table!` macro in arith.rs. The macro generates `dispatch_regular_record`, which `handle` consults before its per-opname match; a Some result stands in for the arm that previously lived in the match. The int_same_as/i>i dormant forward-prep arm joins the table too; it stays absent from the codewriter runtime set (rewrite_op_same_as strips same_as before assembly). t3_audit_opname_gap_inventory now scans arith.rs alongside mod.rs so the relocated arm literals still count as handled. New regular_record_table_is_the_sole_dispatcher asserts the table keys are unique, are runtime opnames (except the dormant same_as), and no longer appear as dead arms in mod.rs. Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit bbfa9ea). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
This was referenced Jul 21, 2026
This was referenced Jul 30, 2026
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
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.
Summary
Mechanically decomposes the
pyre-jit-traceproduction tracerjitcode_dispatch.rs— a single ~37.8k-line file — into a directorymodule with per-cluster submodules, then adds a
macro_rules!pilot thattable-dispatches the uniform record arms. Every step is a pure,
behavior-preserving relocation (moving
fns between files in one cratedoes not change codegen), verified at each milestone.
mod.rsdrops from 37,760 to ~21,795 lines; the rest moves into focusedsubmodules, each re-exported via
pub useso externalcrate::jitcode_dispatch::NAMEpaths stay stable.Commits
jitcode_dispatch.rs→ directory module (mod.rs)#[cfg(test)] mod tests→tests.rsdiag.rsarith.rsbridge_subwalk.rs,heapcache_ops.rs,vable_ops.rsvstack_mirror.rsbranch.rsresume_snapshot.rsHELPER(code, op, ctx, OpCode::VARIANT)dispatch arms for theint_*/float_*/ptr_*record families with aregular_record_table!macro. It generatesdispatch_regular_record,which
handleconsults before its per-opnamematch. This is the Rustanalogue of the
@arguments-decorated exec-generatedopimpl_*families in
pyjitpl.py, which collapse the opcode × register-bankcross product into one metaprogrammed table rather than spelling out
each arm.
Notes
int_same_as/i>idormant forward-prep arm moves into the table too;it stays absent from the codewriter runtime set (
rewrite_op_same_asstrips
same_asbefore assembly).t3_audit_opname_gap_inventorynow scansarith.rsalongsidemod.rsso the relocated arm literals still count as handled.
regular_record_table_is_the_sole_dispatcherasserts the table keysare unique, are runtime opnames (except the dormant
same_as), and nolonger appear as dead arms in
mod.rs.Verification
cargo test -p pyre-jit-trace --features dynasm: 258 + 11 + 1 pass.cargo fmtclean.python pyre/check.py: dynasm 225/225, cranelift 225/225 — the full9-commit refactor is runtime-neutral on both backends.
🤖 Generated with Claude Code
Summary by CodeRabbit