Repository navigation
jit: #621 retire the py-indexed VALUE tables (pcdep_color_slots / result_color_at_pc) - #627
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR retires Python-PC keyed resume color and depth tables, adds JitCode-PC pcdep accessors and color-map metadata, updates bridge and inline-frame reconstruction, simplifies dispatch audit fallbacks, and adds pivot audit data reporting. ChangesJitCode-PC Resume Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GuardResume
participant jitcode_dispatch
participant PyJitCodeMetadata
GuardResume->>jitcode_dispatch: Resolve guard resume coordinate
jitcode_dispatch->>PyJitCodeMetadata: Find residual marker by JitCode-PC
PyJitCodeMetadata-->>jitcode_dispatch: Return resume marker JitCode-PC
jitcode_dispatch->>PyJitCodeMetadata: Read depth and result color
PyJitCodeMetadata-->>jitcode_dispatch: Return marker-keyed metadata
jitcode_dispatch->>jitcode_dispatch: Collect active boxes
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3c0b9b5). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
018e5c3 to
05ed11a
Compare
![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/state.rs`:
- Around line 932-946: Remove the vacuous `legacy`/`twin` comparison in the
`pcdep_for_jitcode_pc` audit block, or replace it with a genuinely distinct
legacy source so divergence remains detectable; do not retain unreachable `"di"`
reporting. Update the nearby documentation to remove references to the retired
`metadata.pcdep_color_slots[0]`, and verify the `pcmap_pivot_audit_enabled`
gated path is reachable under the relevant feature and logging conditions.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 9059-9076: Update pcdep_color_for_slot to accept a jit_pc
directly, rename the parameter accordingly, and remove the py_floor_by_jit_pc
reverse lookup. Pass the supplied jit_pc directly to pcdep_trivia_for_jitcode_pc
and pcdep_for_jitcode_pc, and update the active caller live_pc_containing_all
result to match the corrected parameter semantics.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Line 13315: Update the has_color_map assignment to inspect whether any inner
vector in pcdep_color_slots contains entries, rather than checking only whether
the outer Vec is non-empty; preserve false when all inner vectors are empty and
true when at least one color-map entry exists.
🪄 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: 5e4d2b67-6488-4e2c-bcb3-dda3aa4636a8
📒 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_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
| let legacy = pjc.pcdep_for_jitcode_pc(0); | ||
| if crate::jitcode_dispatch::pcmap_pivot_audit_enabled() { | ||
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "fire"); | ||
| let twin = pjc.pcdep_for_jitcode_pc(0); | ||
| if legacy == twin { | ||
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "eq"); | ||
| } else { | ||
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "di"); | ||
| crate::jitcode_dispatch::pcmap_pivot_audit_record_data( | ||
| "entry_param_colors", | ||
| &format!("legacy={legacy:?} twin={twin:?}"), | ||
| ); | ||
| } | ||
| } | ||
| legacy |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Vacuous self-comparison makes the pivot audit unable to ever report divergence.
legacy and twin are both pjc.pcdep_for_jitcode_pc(0) on an unmodified pjc, so legacy == twin is always true. The "di" branch and pcmap_pivot_audit_record_data(...) are unreachable, and the audit records only "eq" regardless of state. For a parity-critical migration this yields false assurance — a divergence-detection audit that structurally cannot detect divergence.
The old legacy path presumably read the retired Python-PC pcdep_color_slots; with that source gone the comparison should either be removed or wired to the intended distinct source. Also note the doc comment still references metadata.pcdep_color_slots[0], which no longer exists.
Before relying on trace or debug logs, verify that the gated code path is actually reached, including feature flags and other logging conditions.
🩹 Collapse the vacuous comparison
- let legacy = pjc.pcdep_for_jitcode_pc(0);
- if crate::jitcode_dispatch::pcmap_pivot_audit_enabled() {
- crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "fire");
- let twin = pjc.pcdep_for_jitcode_pc(0);
- if legacy == twin {
- crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "eq");
- } else {
- crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "di");
- crate::jitcode_dispatch::pcmap_pivot_audit_record_data(
- "entry_param_colors",
- &format!("legacy={legacy:?} twin={twin:?}"),
- );
- }
- }
- legacy
+ let entries = pjc.pcdep_for_jitcode_pc(0);
+ if crate::jitcode_dispatch::pcmap_pivot_audit_enabled() {
+ crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "fire");
+ }
+ entries📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let legacy = pjc.pcdep_for_jitcode_pc(0); | |
| if crate::jitcode_dispatch::pcmap_pivot_audit_enabled() { | |
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "fire"); | |
| let twin = pjc.pcdep_for_jitcode_pc(0); | |
| if legacy == twin { | |
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "eq"); | |
| } else { | |
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "di"); | |
| crate::jitcode_dispatch::pcmap_pivot_audit_record_data( | |
| "entry_param_colors", | |
| &format!("legacy={legacy:?} twin={twin:?}"), | |
| ); | |
| } | |
| } | |
| legacy | |
| let entries = pjc.pcdep_for_jitcode_pc(0); | |
| if crate::jitcode_dispatch::pcmap_pivot_audit_enabled() { | |
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "fire"); | |
| } | |
| entries |
🤖 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/state.rs` around lines 932 - 946, Remove the vacuous
`legacy`/`twin` comparison in the `pcdep_for_jitcode_pc` audit block, or replace
it with a genuinely distinct legacy source so divergence remains detectable; do
not retain unreachable `"di"` reporting. Update the nearby documentation to
remove references to the retired `metadata.pcdep_color_slots[0]`, and verify the
`pcmap_pivot_audit_enabled` gated path is reachable under the relevant feature
and logging conditions.
Source: Coding guidelines
| /// read from the JitCode-PC twins (local `i` is | ||
| /// slot `i`, operand-stack depth `d` is slot `nlocals + d`). Colors | ||
| /// are per-program-point (chordal coloring may coalesce | ||
| /// disjointly-live slots and re-color across PCs), so there is no | ||
| /// flat slot → color lookup; `None` when the slot carries no live | ||
| /// restorable entry at that PC. | ||
| fn pcdep_color_for_slot(jitcode_index: i32, py_pc: usize, slot: usize) -> Option<u32> { | ||
| pyre_jit_trace::state::pcdep_color_slots_at(jitcode_index, py_pc as i32) | ||
| let payload = pyre_jit_trace::state::pyjitcode_for_jitcode_index(jitcode_index)?; | ||
| let jit_pc = payload | ||
| .metadata | ||
| .py_floor_by_jit_pc | ||
| .iter() | ||
| .find(|&&(_, py)| py as usize == py_pc) | ||
| .map(|&(pc, _)| pc as usize)?; | ||
| payload | ||
| .pcdep_trivia_for_jitcode_pc(jit_pc) | ||
| .map(ToOwned::to_owned) | ||
| .or_else(|| payload.pcdep_for_jitcode_pc(jit_pc))? |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Accept jit_pc directly to avoid reverse lookup and fix type confusion.
The active caller (test_load_local_checked_value_respects_symbolic_local_type_with_compiled_trace_jitcode) passes a resume_pc generated by live_pc_containing_all into this helper. Since live_pc_containing_all returns a jit_pc, passing it into a py_pc parameter is conceptually incorrect. Attempting to reverse-map this jit_pc through py_floor_by_jit_pc as if it were a py_pc only succeeds by coincidence when both offsets are 0.
Since the PR migrates metadata lookups to use jit_pc natively, you can simplify the helper to accept a jit_pc directly and drop the reverse lookup.
♻️ Proposed refactor
- fn pcdep_color_for_slot(jitcode_index: i32, py_pc: usize, slot: usize) -> Option<u32> {
+ fn pcdep_color_for_slot(jitcode_index: i32, jit_pc: usize, slot: usize) -> Option<u32> {
let payload = pyre_jit_trace::state::pyjitcode_for_jitcode_index(jitcode_index)?;
- let jit_pc = payload
- .metadata
- .py_floor_by_jit_pc
- .iter()
- .find(|&&(_, py)| py as usize == py_pc)
- .map(|&(pc, _)| pc as usize)?;
payload
.pcdep_trivia_for_jitcode_pc(jit_pc)
.map(ToOwned::to_owned)
.or_else(|| payload.pcdep_for_jitcode_pc(jit_pc))?
.iter()
.find(|&&(b, _, s)| b == 1 && s as usize == slot)
.map(|&(_, c, _)| u32::from(c))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// read from the JitCode-PC twins (local `i` is | |
| /// slot `i`, operand-stack depth `d` is slot `nlocals + d`). Colors | |
| /// are per-program-point (chordal coloring may coalesce | |
| /// disjointly-live slots and re-color across PCs), so there is no | |
| /// flat slot → color lookup; `None` when the slot carries no live | |
| /// restorable entry at that PC. | |
| fn pcdep_color_for_slot(jitcode_index: i32, py_pc: usize, slot: usize) -> Option<u32> { | |
| pyre_jit_trace::state::pcdep_color_slots_at(jitcode_index, py_pc as i32) | |
| let payload = pyre_jit_trace::state::pyjitcode_for_jitcode_index(jitcode_index)?; | |
| let jit_pc = payload | |
| .metadata | |
| .py_floor_by_jit_pc | |
| .iter() | |
| .find(|&&(_, py)| py as usize == py_pc) | |
| .map(|&(pc, _)| pc as usize)?; | |
| payload | |
| .pcdep_trivia_for_jitcode_pc(jit_pc) | |
| .map(ToOwned::to_owned) | |
| .or_else(|| payload.pcdep_for_jitcode_pc(jit_pc))? | |
| /// read from the JitCode-PC twins (local `i` is | |
| /// slot `i`, operand-stack depth `d` is slot `nlocals + d`). Colors | |
| /// are per-program-point (chordal coloring may coalesce | |
| /// disjointly-live slots and re-color across PCs), so there is no | |
| /// flat slot → color lookup; `None` when the slot carries no live | |
| /// restorable entry at that PC. | |
| fn pcdep_color_for_slot(jitcode_index: i32, jit_pc: usize, slot: usize) -> Option<u32> { | |
| let payload = pyre_jit_trace::state::pyjitcode_for_jitcode_index(jitcode_index)?; | |
| payload | |
| .pcdep_trivia_for_jitcode_pc(jit_pc) | |
| .map(ToOwned::to_owned) | |
| .or_else(|| payload.pcdep_for_jitcode_pc(jit_pc))? | |
| .iter() | |
| .find(|&&(b, _, s)| b == 1 && s as usize == slot) | |
| .map(|&(_, c, _)| u32::from(c)) | |
| } |
🤖 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 9059 - 9076, Update
pcdep_color_for_slot to accept a jit_pc directly, rename the parameter
accordingly, and remove the py_floor_by_jit_pc reverse lookup. Pass the supplied
jit_pc directly to pcdep_trivia_for_jitcode_pc and pcdep_for_jitcode_pc, and
update the active caller live_pc_containing_all result to match the corrected
parameter semantics.
05ed11a to
5226c75
Compare
![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: 5226c75daa
ℹ️ 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 legacy = pjc.pcdep_for_jitcode_pc(0); | ||
| if crate::jitcode_dispatch::pcmap_pivot_audit_enabled() { | ||
| crate::jitcode_dispatch::pcmap_pivot_audit_record_fire("entry_param_colors", "fire"); | ||
| let twin = pjc.pcdep_for_jitcode_pc(0); | ||
| if legacy == twin { |
There was a problem hiding this comment.
When PYRE_PCMAP_PIVOT_AUDIT is enabled, this probe now reads both legacy and twin from pjc.pcdep_for_jitcode_pc(0), so the comparison can only report eq. In audit/census runs for entry parameter color migration, any missing or divergent independent source will be silently recorded as equal, invalidating the results the probe is meant to certify; remove the comparison or compare against a genuinely independent baseline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0237f2e6af
ℹ️ 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 ec_color_names_frame_slot = pyjitcode_for_jitcode_index(frame0.jitcode_index) | ||
| .is_some_and(|payload| { | ||
| payload | ||
| .pcdep_trivia_for_jitcode_pc(frame0.pc as usize) |
There was a problem hiding this comment.
In bridge setup, the semantic mirror above is built from maps.pcdep_entries produced by bridge_semantic_maps_from_pc(frame0.pc), but this collision gate now re-resolves a trivia-skipped pcdep table. When the resume PC is a marker/trivia coordinate whose trivia twin maps portal_ec_reg to a later frame slot while the actual bridge pcdep does not, this skips seeding sym.execution_context from the decoded bridge_registers_r; after clear_active_vable, downstream EC consumers can see OpRef::NONE even though the EC red was present in the bridge register bank. Reuse maps.pcdep_entries (or the same plain coordinate) for this check.
Useful? React with 👍 / 👎.
| } | ||
| let pjc = pyjitcode_for_code(code)?; | ||
| pjc.metadata.pcdep_color_slots.first().cloned() | ||
| let legacy = pjc.pcdep_for_jitcode_pc(0); |
There was a problem hiding this comment.
This helper needs the callee entry PC's color map, but pcdep_by_jit_pc is keyed by emitted JitCode offsets and pcdep_for_jitcode_pc(0) returns None whenever the first decodable entry marker is not byte offset 0. In that case emit_inline_call treats None as an uncolored portal/skeleton and seeds arguments into identity registers (r0..), so colored callees whose entry marker lives later get their params in the wrong registers. Resolve the codewrite-time entry marker (for example merge_entry_for(0)) before querying the twin.
Useful? React with 👍 / 👎.
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
At reachable resume coordinates, walk-visited depth equals static liveness depth because the trace visited that PC. Portal and skeleton payloads have a null code pointer, so the fallback returns zero as the retired empty field did. The corpus measured this fallback as zero-fire on both backends. Assisted-by: Claude Assisted-by: Codex
All 11 carry toggles (PYRE_M73_PEROP/BRANCH/S1MARKER/PFMARKER/ARMPATH/LOOPCLOSE/ LCLIVE/INLCALLER/OUTERCAP_CARRY, PYRE_M369_PFRAME_CARRY, PYRE_M366_NONBRANCH_PC) defaulted ON. Delete the env-var helper fns and collapse each of the 18 call sites in jitcode_dispatch.rs and trace_opcode.rs to the ON branch; the result is byte-identical to the prior default runtime. The removed OFF branches were the only readers of the gate-off fallback that indexed the py-VALUE tables deleted earlier in this branch. Refresh the two collect_outer_active_boxes fallback-arm comments (no gate-off configuration remains). Assisted-by: Claude Assisted-by: Codex
compute_inline_caller_frame, compute_nested_inline_caller_frame, and the FBW walker result-color read now resolve the pending-result register color via result_color_trivia_for_jitcode_pc (exact-marker precedence, matching the adjacent depth_trivia_for_jitcode_pc read) instead of result_color_for_jitcode_pc_pred, whose result_color_by_jit_pc map kept only the first Python PC per shared JitCode offset and could return a stale color when trivia-folded PCs share a resume marker. Retire the now-unused result_color_by_jit_pc field, its accessor, its fixture inits, and its codewriter construction; block_head_py_by_jit_pc keeps the shared loop. Assisted-by: Claude Assisted-by: Codex
0237f2e to
3c0b9b5
Compare
Closes #621.
After #602 made the resume coordinate jitcode-native,
PyJitCodeMetadatastill carried three py-indexed side tables:pcdep_color_slots,result_color_at_pc, anddepth_at_py_pc. This PR retires every remaining py-keyed consumer under the established census discipline and deletes all three; their jitcode-keyed twins (and, for depth, the permitted liveness-crate static analysis) are now the only runtime sources.Layer A — carried-coordinate twin flips (census-first)
inject_root_call_result,reconstruct_inline_recipepending result color). Corpus census (both backends): the one firing site (recipe pcdep, 1293 fires) was 100% equal to the trivia twin; 0 divergences.Layer B — residual consumers + color-table deletion
cjc < 0guard-pcdep fallback and the inline-caller marker-miss arms (all 0-fire) retire to no-metadata / early-decline;sub_jitcode_entry_param_colorsreads the predecessor twin at offset 0 (986/986 equal).reconstruct_inline_recipeand thesetup_bridge_symec-seed gate indexed py-keyed tables with jitcode offsets (wrong key space). The bridge-maps read returned(depth 0, empty pcdep)on every corpus fire (500/500 divergent vs the twin), silently forcing the multi-frame recipe path to decline; the ec gate spuriously skipped ec seeding once per corpus run. Both now key by the carried jitcode coordinate.pcdep_color_slots+result_color_at_pcremoved;has_color_mapis now a codewriter-published bool replacing the emptiness test.Tail —
depth_at_py_pcfield deletionor_else(structurally dead: coupled 1:1 with the pcdep twin that already declines), and thevia_py_pcbridge fallback (0-fire in corpus) — are retired.via_py_pcnow sources depth from the issue-permitted liveness static analysis (null-code_ptrguarded → 0 for portal/skeleton); at any reachable resume coordinate the walk-visited depth equals the static depth. The uncalledbridge_semantic_maps_atNO_JITCODE wrapper is also deleted.The liveness-crate method
liveness_for(code).depth_at_py_pc()(CodeObject static analysis, CPython-derivable) is unchanged, as the issue specifies.Verification
main(jit: P2 bridge compile leg default-ON + guard-thrash fixes + vstack-mirror classification #607 P2 bridge compile, Interpreter correctness: attribute/error-message parity, str width #614 interpreter correctness);inject_root_call_resultconflicts resolved by blending jit: P2 bridge compile leg default-ON + guard-thrash fixes + vstack-mirror classification #607'sresidual_ref_call_dst_beforeprimary with the twin fallback.python3 pyre/check.py --backend dynasm/--backend cranelift: 217/217, audit-off and pivot/result-audit-on (no panic).cargo test -p pyre-jit-trace -p pyre-jit --release(dynasm): all green.cargo fmt --checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Diagnostics