Repository navigation
jit: #73 Layer-2 + pc_map retirement — retire model/carrier Python-pc channels and the runtime py→jit inversion table (S1–S7, W1–W3b) - #602
Conversation
WalkthroughThe PR refactors JIT dispatch and resume handling around native JIT-code coordinates, introduces floor-segment PC metadata, updates FOR_ITER and inline-abort tracking, and expands PC-map audits and tests. ChangesJIT coordinate and resume refactor
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)pyre/pyre-jit-trace/src/jitcode_dispatch.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ca843b7). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
9072758 to
67e1155
Compare
67e1155 to
c263df0
Compare
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
The per-opcode arm entry seam has no call sites; the production tracer dispatches the full-body JitCode cursor via dispatch_via_miframe. Its collect_outer_active_boxes call was the last entry caller without a carried JitCode coordinate (0 fires in the entry-audit census on both backends). Assisted-by: Claude
Assisted-by: Claude Assisted-by: Codex
Carry the CALL JitCode coordinate through the nested-abort latch and derive the Python pc at the interpreter-flush boundary. The phase-1 corpus audit did not exercise this path; the derivation relocates the unchanged producer inversion. Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Store raw producing JitCode coordinates in WalkContext where available and derive the unchanged backxlat value at readers. The phase-1 corpus does not exercise those readers. Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Inline-abort carriers now store JitCode coordinates and derive Python words at the interpreter-flush boundary. The corpus exercises the Entry leg (22 resolution fires per backend); the MidBody leg remains corpus-unreachable and flips by relocated inversion. Unresolvable identities decline to legacy replay. Assisted-by: Claude Assisted-by: Codex
Carry the producing JitCode source beside the legacy body pc and census boundary re-derivation without changing matching behavior. Assisted-by: Claude Assisted-by: Codex
Store FOR_ITER body coordinates natively and resolve them only at stash match points, declining unresolved entries conservatively. Assisted-by: Claude Assisted-by: Codex
Add an additive parent-resume coordinate and report-only audits for its native consumers and builder metadata twins. Assisted-by: Claude Assisted-by: Codex
Remove the paused parent Python resume word, resolve its coordinates at native consumers, and switch certified metadata readers to JitCode twins. Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
The per-opcode arm entry dispatch_via_miframe_at_opcode_entry was deleted as dead code; nine surviving doc comments still cited it as a live function. Reword them to mark the entry as retired and describe the current seeding sites (inline CALL-site capture, full-body guard capture, env fallback for store_subscr_fn_addr). Assisted-by: Claude
Build the Python-PC inverse as a JitCode-PC boundary table and audit it against the retained legacy resolver. Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
Assisted-by: Claude Assisted-by: Codex
b578952 to
ca843b7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca843b7188
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let depth = match resume_marker_jit_pc { | ||
| Some(marker) => unsafe { &(*caller_sym.jitcode).payload } | ||
| .depth_trivia_for_jitcode_pc(marker) | ||
| .unwrap_or(0) as usize, |
There was a problem hiding this comment.
Keep caller fallthrough data keyed by the fallthrough PC
When after_residual_marker_for_jitcode_pc returns a -live- marker shared by several Python PCs in the same marker run, these marker-keyed lookups resolve data for the marker's first/block-head Python PC, not for the CALL fallthrough computed in fallthrough_py_pc. The result-color twin is explicitly built as the first Python PC per shared marker and marked audit-only in pyjitcode.rs, so inline calls whose return point is later in the same marker run can size the paused caller frame and null the call-result register using the wrong opcode's metadata, corrupting multi-frame guard resumes. Please keep the fallthrough-path metadata keyed by fallthrough_py_pc or add a fallthrough-specific JitCode-PC twin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 233-240: Unify carried CALL-coordinate resolution across the
outer-resume branch and the other referenced branches using one validated,
normalized resolver. Require the resolved JIT code to have non-empty drained
metadata before calling python_pc_for_jitcode_pc, reject mappings that resolve
to PC 0, and apply skip_python_trivia_forward consistently. Preserve the
entry-carrier duplicate check when normalized coordinates represent the same
CALL, returning Some only after validation and normalization.
🪄 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: 11f361a8-34db-4772-94b8-357e513deade
📒 Files selected for processing (6)
pyre/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/jit/codewriter.rs
| let outer = crate::state::pyjitcode_for_jitcode_index(outer_jitcode_index as i32); | ||
| let resolved = outer | ||
| .as_ref() | ||
| .is_some_and(|payload| !payload.code_ptr.is_null()); | ||
| audit_midbody_carrier_resolution("entry_flush_call_py_pc", resolved); | ||
| let outer = outer.filter(|payload| !payload.code_ptr.is_null())?; | ||
| let call_py_pc = | ||
| crate::jitcode_dispatch::python_pc_for_jitcode_pc(&outer.metadata, call_jitcode_pc) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one validated, normalized resolver for every carried CALL coordinate.
A non-null code_ptr does not prove usable metadata: python_pc_for_jitcode_pc returns 0 when py_floor_by_jit_pc is empty. The outer-resume branch also skips neither this validation nor skip_python_trivia_forward, so it can commit at PC 0 or miss the entry-carrier duplicate check when both coordinates represent the same CALL.
Require drained/non-empty metadata and normalize CALL PCs consistently before returning Some.
Also applies to: 258-272, 2159-2182
🤖 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 233 - 240, Unify carried
CALL-coordinate resolution across the outer-resume branch and the other
referenced branches using one validated, normalized resolver. Require the
resolved JIT code to have non-empty drained metadata before calling
python_pc_for_jitcode_pc, reject mappings that resolve to PC 0, and apply
skip_python_trivia_forward consistently. Preserve the entry-carrier duplicate
check when normalized coordinates represent the same CALL, returning Some only
after validation and normalization.
Continues the #73 pc_map-removal epic (follow-up to #589). Layer-2 slices S1–S6: every remaining model/carrier Python-pc channel in the walker is deleted; the Python coordinate is now derived once at the interpreter-flush boundary from a carried JitCode coordinate, or replaced outright by a certified jitcode-keyed twin read.
S1 —
InlineParentFrame.call_py_pc(2 commits): threadcall_jitcode_pcthrough every producer, then delete the py field; the flush boundary derives py via the samepython_pc_for_jitcode_pcthe producers ran (pure code motion). Unresolvablejitcode_indexdeclines the flush defensively.S2 —
WalkContext.entry_py_pc(2 commits): replaced byEntryPyPc { Py(u32), Jit(usize) }; two eagerbackxlat_py_pcproducer calls and one inline inversion expansion deleted. A marker-derived flip was considered and rejected (raw producing coordinate carried instead — census precedent shows resume markers diverge from trivia-normalized coordinates).S3 — inline-abort carrier py fields (2 commits):
InlineAbortCarrier/MidBodyPayloadlosecall_py_pc/post_call_py_pc/callee_py_pc; the eager outer-CALL inversion block becomes a carried(jitcode_index, op.pc)coordinate resolved at the trace.rs flush boundary. Entry leg observed live (22 fires per backend, 0 divergence).S4 — FOR_ITER inflight
body_pc(2 commits):InflightForiterBody { Py, Jit }; thefbw_foriter_body_pc_from_op_pchelper and its 3 producer calls deleted; resolution happens just before the keyed lookup at all 5 delivery sites (203 resolved fires, 0 divergence). Resolution failure = no match (conservative refuse-to-deliver; legacy bypass runs the iteration once).S5 —
InlineParentFrame.resume_py_pc(2 commits): replaced by a requiredParentResumeCoord { Backxlat, CallFallthrough }; the two vable consumers derive at the boundary, and 4 py-keyed table reads flip to their certified jitcode-keyed twins (depth_for_jitcode_pc_pred,pcdep_for_jitcode_pc,depth_trivia_for_jitcode_pc,result_color_for_jitcode_pc_pred) — census 6,164/6,164 equal across both backends. Missing marker falls back to legacy py; missing metadata declines withGuardResumeCoordinateUnavailable.S6 — boundary dedup + frontier censuses (2 commits): the duplicated WALK_END restart-pc inversion in trace.rs is derived once; report-only censuses for the recipe result-color read (0 fires — gate left in place) and the multi-frame callee diagnostic inversion (diagnostic-only; census re-confirmed plain resume marker ≠ trivia-normalized coordinate, 89/1,673 divergence on both backends).
With this the mechanical Layer-2 frontier is exhausted: no model or carrier struct stores a Python pc. Remaining py reads are boundary derivations (single inversion points) plus the
vstack_cur_pypcmirror, which retires with the walker-as-tracer core (Layer 3).Verification per slice: codex-implemented, independently re-verified — check.py dynasm 208/208 + cranelift 208/208 (audit-off), audit-on runs green (
PYRE_PCMAP_*_AUDIT),cargo test -p pyre-jit-trace -p pyre-jitstandalone green. Only defensive declines added; no panic paths.S7 — guard-capture py narrowing (3 commits): the dead
resume_py_pcstamp and theencode_after_residual_call_pcpy-word encode are deleted; three guard-capture depth reads flip to the trivia/marker jitcode-keyed twins (censuses 5,198+5,566 and 579+649 comparisons, all equal; post-flip corpus audits 9,089+1,193 / 9,707+1,311 all equal); the MidBody producer depth/pcdep reads flip to their predecessor twins.W1 — jitcode-keyed pivot table + resolver rewire (1 commit):
py_floor_by_jit_pc— a piecewise-constant boundary table (jit start → containing Python pc) built at metadata finalize — replaces the runtime inversion implementation ofpython_pc_for_jitcode_pc, flipping all ~45 call sites at once. Certified by an exhaustive build-time sweep underPYRE_PCMAP_PIVOT_AUDIT: 684 JitCodes / 859,032 jitcode bytes, pivot == legacy at every byte, both backends.W2 — floor-only refactor + direct-consumer flips (3 commits): the pivot is reduced to floor boundaries (block-head precedence stays in
block_head_py_by_jit_pc);vstack_containing_py_pcand the two MidBody anchors flip to floor-segment equivalents (0 divergence); a report-only census instruments the two guard-snapshot entry-anchor legs.W3a — entry-anchor completion + residue censuses (3 commits): the guard-arm entry anchor flips to the floor-segment start (census 435/612 fires, 0 divergence);
n_py_instrsreplaces the four length-only table reads; the fallthrough leg gets an inverse-query candidate census whose miss set provably coincides with the legacy-None set (26/26 per backend, count arithmetic), plus fire counters on every remaining fallback arm (all zero except the 26-fire py-value entry arms).W3b — runtime inversion-table deletion (2 commits, net −452 lines): the fallthrough entry anchor flips to the inverse query;
PyJitCodeMetadata.first_jit_pc_by_py_pcandcarryfwd_resume_pcare deleted along withpython_pc_for_jitcode_pc_legacy,pc_map_marker_for, the legacy anchor helpers, and the audit oracles. The codewriter keeps a build-time local first-op array to construct the pivot and twins. The runtime now holds no Python-pc→jitcode inversion table and no runtime inversion search; Python coordinates resolve via the jitcode-keyedpy_floor_by_jit_pc/block_head_py_by_jit_pctables — the same shape as PyPy baking Python pcs into jitcode as constants.Verification for S7/W1–W3b: same per-slice battery (both backends audit-off +
PYRE_PCMAP_PIVOT_AUDIT=1audit-on, corpus 208–210/210 green throughout, MidBody workloads, cargo test with the known standalone-flake rerun).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Diagnostics