Repository navigation
jit: #73 — delete the py_pc↔JitCode resume-coordinate translation; author guard resume data in JitCode coordinates - #569
Conversation
WalkthroughThe change replaces Python-PC resume translation with carried JitCode-coordinate handling across metadata generation, guard dispatch, trace walks, frame reconstruction, and deoptimization. Invalid or unavailable coordinates now decline explicitly, while marker-based liveness, result-color audits, and walk-end restart handling are added. ChangesResume coordinate migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 20b6620615
ℹ️ 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".
| .loop_close_marker_jit_pc | ||
| .filter(|_| crate::jitcode_dispatch::m73_lclive_carry_enabled()) | ||
| { | ||
| Some(jit_pc) => Some(jit_pc), | ||
| None => { |
There was a problem hiding this comment.
For any populated jitcode guard captured through current_fail_args/generate_guard outside close_loop_args_at, loop_close_marker_jit_pc is never set (the production setter is the loop-close path), so this branch takes the None arm below, calls request_trace_abort(), and returns no active boxes. That makes ordinary guards such as GuardClass, GuardTrue, or GuardNoException abort the trace instead of recording resume data unless the capture happens during loop close, effectively disabling the trait tracer for normal guarded code.
Useful? React with 👍 / 👎.
| if used_carried { | ||
| Some(carried as usize) | ||
| } else { | ||
| self.resolve_resume_pc(raw_pc) | ||
| None |
There was a problem hiding this comment.
When the carried word is NO_JITCODE_PC or otherwise not a decodable -live- offset, this now returns None, but several guard-snapshot callers still pass the sentinel and then fall back with unwrap_or(ctx.entry_py_pc) (for example the nonstandard-vable and arm-path captures). Since blackhole_resume_via_rd_numb was changed to reject non-decodable frame PCs instead of translating them, those sentinel paths can publish a raw Python PC into resume data that later deopts cannot rebuild; the callers need to decline before capture or supply a real carried marker.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b3e561f). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
result_color_at_pc was the last py_pc-keyed resume-metadata table without a JitCode-pc-keyed twin (depth_pred_by_jit_pc and block_head_py_by_jit_pc already have theirs). Derive result_color_by_jit_pc beside block_head_py_by_jit_pc in finalize_jitcode from the same pc_map_bytes first-seen offsets, add the predecessor accessor result_color_for_jitcode_pc_pred, and assert equality with the py_pc table at the three inline/bridge seams that already hold a genuine JitCode offset, behind PYRE_PCMAP_RESULT_AUDIT. No consumer is switched — result_color_at_pc stays the runtime source of truth — so the build is byte-identical with the gate off. This completes the metadata-twin inventory that the eventual walker-jitcode-pc coordinate flip needs; it does not itself change resume behaviour. check.py 181/181 dynasm + 181/181 cranelift audit-off; PYRE_PCMAP_RESULT_AUDIT=1 synth corpus 0 assertion failures on both backends. Assisted-by: Claude Assisted-by: Codex
…ume_pc py_pc translation Assisted-by: Claude Assisted-by: Codex
…t, delete the py_pc table Assisted-by: Claude Assisted-by: Codex
…jitcode-native resume coordinates The tests fed a py_pc resume coordinate and left the carried resume marker unset while production reads boxes at the JitCode MIFrame resume marker. Migrate the fixtures and test-only plain liveness accessor to JitCode-native coordinates, and seed the marker through a test-only setter. The production resolver path is unchanged; the trait-leg decline was proven production-unreachable by a 190-file corpus census. Assisted-by: Claude Assisted-by: Codex
…get_list_of_active_boxes unit fixtures The migration made `get_list_of_active_boxes` read the carried JitCode resume marker plus the `pcdep_by_jit_pc` / `depth_pred_by_jit_pc` twins. Two `pyre-jit-trace` unit fixtures built a MIFrame with `loop_close_marker_jit_pc = None` and seeded only the py_pc-keyed `pcdep_color_slots` / `depth_at_py_pc`, so the snapshot declined to an empty box list (first fixture) or resolved the coalesced stack color to the wrong register (second fixture). Seed the resume marker at JitCode offset 0 and mirror the color/depth tables into the JitCode twins at the same offset. Assisted-by: Claude
Record and consume the full-body walk restart PC when a loop marker lands inside a super-instruction. Decline walks whose entry stack depth disagrees with the carried snapshot, and advance an interpreter handoff only when liveness proves its restored stack belongs to the following opcode. Assisted-by: Claude
Decline a depth-mismatched full-body walk only when its carried JitCode coordinate is the resume marker itself. Interior branch resumes retain their own coordinate and continue walking. Assisted-by: Claude
The resume_jitcode_pc_for deletion and the 3-argument select_recipe_entry migration left two callers in the multiframe bridge-drain path still referencing the removed py_pc translation: - the depth-2 middle-frame select_recipe_entry call passed the inverted py_pc plus a resume_jitcode_pc_for-derived closure; pass the carried middle.jitcode_pc directly, matching drive_bridge_carrier_walk. - recipe_parent_frame_from_recipe translated the inverted py_pc via resume_jitcode_pc_for when the recipe pc was not a resolved JitCode offset; decline (return None) in that case instead. Assisted-by: Claude
20b6620 to
034e340
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 034e340ea4
ℹ️ 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".
| } else if let Some(restart_pc) = walk_end_restart_pc { | ||
| frame.set_last_instr_from_next_instr(restart_pc); |
There was a problem hiding this comment.
When a full-body walk closes a loop but flush_walk_end_state_to_frame declines (for example because PYRE_FBW_END_FLUSH=0 or fbw_has_unjournaled_effect() is true), the live frame is still at the trace-entry state; this else if nevertheless advances last_instr to the loop/restart PC without restoring the walked locals/stack. That skips replay of the loop body while keeping pre-walk frame contents, corrupting interpreter fallback results; the same pattern appears in the bound_reached tracing site below.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/pyjitcode.rs (1)
678-709: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_raw_pcparameter is now dead weight.Since
resolve_resume_pc_with_jitcode_pcno longer consults_raw_pcat all (only the carried coordinate matters), consider dropping the parameter from the signature and updating both call sites, rather than carrying an always-ignored argument indefinitely.🤖 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/pyjitcode.rs` around lines 678 - 709, Remove the unused _raw_pc parameter from resolve_resume_pc_with_jitcode_pc and update both callers to pass only carried and op_live. Preserve the existing carried-coordinate expansion and validation behavior.pyre/pyre-jit-trace/src/jitcode_dispatch.rs (1)
12994-13030: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
GuardResumeCoordinateUnavailable.pcdoesn't identify which frame failed.In
walker_capture_multi_frame_inline_snapshot, both the per-parent-frame resolution failure (inside thefor pf in &parent_framesloop, line ~3015) and the callee's own resolution failure (line ~3029) reportDispatchError::GuardResumeCoordinateUnavailable { pc: callee_op_pc }. When a parent frame'sresolve_resume_pc_with_jitcode_pcfails, the reportedpcis still the innermost callee's op_pc, not the parent frame'spf.jitcode_index/pf.resume_py_pcthat actually failed to resolve — this will mislead debugging of exactly the new decline path this PR wants surfaced.🩹 Proposed fix: attribute the failure to the actual frame
- let Some(pf_pc_word) = crate::state::pyjitcode_for_jitcode_index(pf.jitcode_index as i32) - .and_then(|payload| { - payload.resolve_resume_pc_with_jitcode_pc( - pf.resume_py_pc as i32, - pf_word, - crate::state::op_live(), - ) - }) - .map(|offset| offset as u32) - else { - return Err(DispatchError::GuardResumeCoordinateUnavailable { pc: callee_op_pc }); - }; + let Some(pf_pc_word) = crate::state::pyjitcode_for_jitcode_index(pf.jitcode_index as i32) + .and_then(|payload| { + payload.resolve_resume_pc_with_jitcode_pc( + pf.resume_py_pc as i32, + pf_word, + crate::state::op_live(), + ) + }) + .map(|offset| offset as u32) + else { + return Err(DispatchError::GuardResumeCoordinateUnavailable { pc: pf.resume_py_pc as usize }); + };🤖 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.rs` around lines 12994 - 13030, Update the error handling in walker_capture_multi_frame_inline_snapshot so parent-frame resolution failures inside the for pf in &parent_frames loop report the failing parent frame’s resume coordinate, while the separate callee resolution failure continues to report callee_op_pc. Ensure GuardResumeCoordinateUnavailable.pc identifies the frame whose resolve_resume_pc_with_jitcode_pc call failed.
🤖 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/trace.rs`:
- Around line 1807-1819: Extract the duplicated loop-header marker adjacency
correction into a shared helper near the CloseLoop handling, including
marker-to-Python-PC translation, merge-entry validation, and the fallback
adjustment. Use this helper for both the restart_pc and end-flush header_pc
computations, and verify the fallback advances to the correct next instruction
for variable-width bytecode rather than assuming loop_header_pc + 1.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 5862-5871: Factor the duplicated walk-end resume handling from
jit_merge_point_hook and this call site into a shared helper such as
apply_walk_end_resume. The helper must preserve flush-first precedence,
restoring from executed_frame when walk_end_flushed is true and otherwise
applying walk_end_restart_pc when present; replace both inline blocks with calls
to it.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 12994-13030: Update the error handling in
walker_capture_multi_frame_inline_snapshot so parent-frame resolution failures
inside the for pf in &parent_frames loop report the failing parent frame’s
resume coordinate, while the separate callee resolution failure continues to
report callee_op_pc. Ensure GuardResumeCoordinateUnavailable.pc identifies the
frame whose resolve_resume_pc_with_jitcode_pc call failed.
In `@pyre/pyre-jit-trace/src/pyjitcode.rs`:
- Around line 678-709: Remove the unused _raw_pc parameter from
resolve_resume_pc_with_jitcode_pc and update both callers to pass only carried
and op_live. Preserve the existing carried-coordinate expansion and validation
behavior.
🪄 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: 9e4a4a8d-498b-4e3f-990c-a00425e1de4f
📒 Files selected for processing (10)
majit/majit-ir/src/resumedata.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/pyjitcode.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/rework.md
| let restart_pc = loop_header_marker_jit_pc.map_or(loop_header_pc, |marker| { | ||
| let marker_py = | ||
| crate::jitcode_dispatch::python_pc_for_jitcode_pc(&pjc.metadata, marker) | ||
| as usize; | ||
| if marker_py == loop_header_pc | ||
| && pjc.merge_entry_for(loop_header_pc) != Some(marker) | ||
| { | ||
| loop_header_pc + 1 | ||
| } else { | ||
| marker_py | ||
| } | ||
| }); | ||
| WALK_END_RESTART_PC.with(|c| c.set(Some(restart_pc))); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated marker-adjacency correction; consider extracting a shared helper.
The loop_header_marker_jit_pc.map_or(...) adjacency-correction block (translate marker → python pc, then bump by 1 when the marker's python pc collides with the loop header but isn't the actual merge entry) is repeated verbatim between the restart_pc computation and the end-flush header_pc computation. Both read the same CloseLoop fields, so they stay in sync today, but any future edit to one copy risks silently diverging from the other.
Additionally, the loop_header_pc + 1 fallback assumes the marker's mapped python pc is always exactly one unit past the loop header — worth double-checking against variable-width bytecode (e.g. EXTENDED_ARG-prefixed instructions) at the loop header.
♻️ Proposed extraction
+fn restart_py_pc_for_loop_close(
+ pjc: &crate::PyJitCode,
+ loop_header_pc: usize,
+ loop_header_marker_jit_pc: Option<usize>,
+) -> usize {
+ loop_header_marker_jit_pc.map_or(loop_header_pc, |marker| {
+ let marker_py =
+ crate::jitcode_dispatch::python_pc_for_jitcode_pc(&pjc.metadata, marker) as usize;
+ if marker_py == loop_header_pc && pjc.merge_entry_for(loop_header_pc) != Some(marker) {
+ loop_header_pc + 1
+ } else {
+ marker_py
+ }
+ })
+}Then call this helper at both the restart_pc and header_pc sites.
Also applies to: 1848-1863
🤖 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/trace.rs` around lines 1807 - 1819, Extract the
duplicated loop-header marker adjacency correction into a shared helper near the
CloseLoop handling, including marker-to-Python-PC translation, merge-entry
validation, and the fallback adjustment. Use this helper for both the restart_pc
and end-flush header_pc computations, and verify the fallback advances to the
correct next instruction for variable-width bytecode rather than assuming
loop_header_pc + 1.
| let walk_end_flushed = pyre_jit_trace::trace::take_walk_end_flush_committed(); | ||
| let walk_end_restart_pc = pyre_jit_trace::trace::take_walk_end_restart_pc(); | ||
| if walk_end_flushed { | ||
| frame_root | ||
| .frame() | ||
| .restore_resume_state_from(&executed_frame); | ||
| } else if let Some(restart_pc) = walk_end_restart_pc { | ||
| frame_root | ||
| .frame() | ||
| .set_last_instr_from_next_instr(restart_pc); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Same restart-pc/flush pattern duplicated from jit_merge_point_hook.
This mirrors the walk_end_flushed / walk_end_restart_pc handling added in jit_merge_point_hook (lines 5031-5037) verbatim. Since both call sites must apply the identical "flush wins, else restart_pc" precedence, consider factoring this into a small helper (e.g. fn apply_walk_end_resume(frame: &mut PyFrame, executed_frame: &FrameBox)) to avoid the two copies drifting apart on a future change.
🤖 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/eval.rs` around lines 5862 - 5871, Factor the duplicated
walk-end resume handling from jit_merge_point_hook and this call site into a
shared helper such as apply_walk_end_resume. The helper must preserve
flush-first precedence, restoring from executed_frame when walk_end_flushed is
true and otherwise applying walk_end_restart_pc when present; replace both
inline blocks with calls to it.
…em (#594) * jit: delete the orphaned m73 entry-carry gates #569 removed the only callers of m73_entry_carry_enabled and m73_entry_decline_enabled when it made select_recipe_entry carried-only, leaving both gate functions in place. A repo-wide census finds no reference to either name or to PYRE_M73_ENTRY_CARRY / PYRE_M73_ENTRY_DECLINE outside the two definitions: no caller, and no doc, test, script or CI consumer. With no call site the documented "default ON, =0 opts out" switches already have no effect, so removing them is not a behavior change. Assisted-by: Claude * jit: delete three more orphaned env gates and correct the docs that cite them bridge_audit_enabled, m73_marker_carry_enabled and m73_armarker_carry_enabled have no callers anywhere in the repo: only their own definitions and doc comments mention them or their env vars. With no call site, the documented "default ON, =0 opts out" switches already have no effect, so removing them is not a behavior change. bridge_audit_enabled is the stranded half of the audit pair — the assertion it gated is no longer in the tree, while its sibling result_color_audit_enabled keeps its four callers. Four doc comments still advertised PYRE_PCMAP_BRIDGE_AUDIT as certifying that the predecessor-keyed pcdep_by_jit_pc / depth_pred_by_jit_pc twins equal their py_pc-indexed counterparts. Drop that claim and state the basis those comments already gave: both sides derive from the same resume-marker / first_jit coordinates at codewrite time, so the equality holds by construction. Assisted-by: Claude
This PR is the #73 "walker-as-tracer" break-through: it deletes pyre's legacy py_pc↔JitCode resume-coordinate translation and authors/decodes JIT guard resume data directly in JitCode coordinates. Guard resume, liveness, and box-capture now resolve from carried JitCode markers/twins; the only surviving Python-pc consumer inside
get_list_of_active_boxesis retired onto audit-certified JitCode twins. Net change is roughly +394 / −504 lines across the trace, dispatch, and metadata layers.What changes
155accc(JIT #73: retire the carried jitcode_pc resume side-channel (Artifact 3) #369 base twin, audit-only): Add JitCode-pc-keyed twinPyJitCodeMetadata::result_color_by_jit_pcderived unconditionally in theCodeWriterfinalize path alongside the existing py_pc-keyedresult_color_at_pc(still source of truth), plus predecessor accessorPyJitCode::result_color_for_jitcode_pc_pred. No runtime consumer is switched; fourassert_eq!twin/py_pc equality blocks (three injitcode_dispatch.rs, one intrace.rs) are gated behind a newresult_color_audit_enabled()readingPYRE_PCMAP_RESULT_AUDIT.7732243(delete the translation): DeletePyJitCode::resume_jitcode_pc_for,resolve_resume_pc, and the privatecarryfwd_resume_pc_for, and rewire every caller to the carried JitCode coordinate or to decline/abort/None. AddsDispatchError::GuardResumeCoordinateUnavailable { pc };resolve_resume_pc_with_jitcode_pc's non-carried branch now yieldsNone;select_recipe_entrydrops itspy_pc+derivedparams; severaltrace_opcode.rssnapshot builders nowrequest_trace_abort()instead of publishing an invented resume coordinate.256f35d(post-call catch marker re-key): ReplacePyJitCodeMetadata::after_residual_call_resume_pcand its accessorafter_residual_call_resume_pc_forwith two JitCode-offset-keyed twinsafter_residual_call_resume_marker_by_jit_pc/after_residual_call_resume_pred_by_jit_pcandafter_residual_call_resume_for_jitcode_pc(exact + predecessor tiers).marker_aware_resume_pc/marker_aware_parent_resume_pcdrop the bit-14 marker-encoding path (the latter loses itsparent_jitcode_indexparam) and abort on the py_pc-only path.20b6620(jitcode-native box capture + test migration): In productionget_list_of_active_boxes, delete thelive_pcbinding and source depth/pcdep/result-color fromdepth_for_jitcode_pc_pred/pcdep_for_jitcode_pc/result_color_for_jitcode_pc_predat the carriedresume_jit_pc(replacingdepth_at_py_pc/pcdep_color_slots/result_color_at_pcatlive_pc); a missing carried marker now fails loud via.expect(...)(a panic — the residual no-marker path is proven production-unreachable). Adds cfg-gatedMIFrame::set_resume_marker_for_test, widenspc_map_marker_fortopub(crate), and migrates theeval.rs/state.rsfixtures and helpers to seed and query by JitCode-live-offsets.Why (PyPy parity)
In PyPy,
MIFrame.pcis a JitCode byte position,setup_resume_at_opis the identity, andget_list_of_active_boxesreads live registers atself.pc(JitCode) — there is no Python pc anywhere in the resume / liveness / box-capture path. pyre'sresume_jitcode_pc_for+resolve_resume_pcwere a lossy py_pc↔JitCode translation artifact with no orthodox counterpart. This PR deletes that translation and makes guard resume data JitCode-native end to end, matching the PyPy resume model.Production-safety of the decline
After deleting the normal-case py_pc fallback,
get_list_of_active_boxesresolves the resume coordinate from a carried JitCode marker/twin; the try-block post-call-marker sub-case declines viarequest_trace_abort(early-return), and the residual no-marker path is a fail-loud.expect(...)panic. That path was proven production-unreachable by a targeted census: with all decline sites instrumented, the full 190-file corpus (13 bench + 177 synth) run under the census gate produced zero decline events, while the corpus compiles thousands of loops and fires tens of thousands of guards (loops_aborted=0on every bench). The marker/twin path resolves 100% of production; only the syntheticMIFrame::from_symunit-test fixture lacked a marker, which the final commit seeds via the cfg-gated test-only setter — faithful to production, not masking.Verification
pyre-jiteval::tests: 31 passed / 0 failed.pyre-jitcrate: 306 passed / 0 failed, plus GC-stress 6 / 0.check.pyboth backends: dynasm 187 passed + 1 failed, cranelift 187 passed + 1 failed. The sole failure issynth/list_insert_pop_index"cpython/pypy output mismatch" — a documented pre-existing BASEFAIL (oracle disagreement that occurs before JIT runs), structurally not a JIT regression.Notes
The
155acccresult_color_by_jit_pctable is the #369 audit-only jitcode-keyed metadata twin (no consumer switched) and serves as the verified precondition/base under the #73 deletions in the following commits.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation