jit: eliminate the guard-fail resume-decode backxlat inverse (jitcode-blackhole Slice 3') - #727
Conversation
…jitcode-blackhole Slice 3' Phase A) Add a per-frame Python-pc word to the resume-data frame header, sourced forward from the codewriter `(jitcode_pc, py_pc)` marker pair at guard capture, rather than derived backward via `backxlat_py_pc` at decode. Thread `py_pc` through `recorder::SnapshotFrame`, `resume::SnapshotFrame`, `FrameInfoBuilder`/`push_frame`, and `RebuiltFrame`; write it in all three frame-header encoders (`number()` and the two compact `rd_numb` encoders) and consume it in both decoders (`rebuild_from_numbering` and the blackhole `read_jitcode_pos_pc`). Wire order is `jitcode_index, pc, py_pc, [boxes]` uniformly. Synthetic single frames use the `-1` sentinel. `build_resumed_frames` still derives the value via `backxlat_py_pc`; under `PYRE_M73_PYPC_FWD_AUDIT` it asserts the forward word equals the backxlat result for every frame. Off by default, so production decode is byte-identical. Consumer cutover + backxlat-call-site deletion is Phase B. Remove the dead `ResumedFrame.rd_numb_pc` field (no readers repo-wide). check.py: dynasm 248/248 + cranelift 248/248 with the audit off (byte-identical) and with `PYRE_M73_PYPC_FWD_AUDIT=1` (zero divergence). Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 20 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 Plus Run ID: 📒 Files selected for processing (6)
WalkthroughPer-frame forward Python PCs are added to JIT metadata, snapshots, resume serialization, decoding, and guard-failure reconstruction. Snapshot capture APIs and frame layouts are extended, while parity tests cover the expanded encoded format and sentinel behavior. ChangesForward Python PC resume flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JITCode
participant SnapshotCapture
participant ResumeEncoder
participant ResumeDecoder
participant FrameRebuilder
JITCode->>SnapshotCapture: resolve forward py_pc
SnapshotCapture->>ResumeEncoder: provide SnapshotFrame with py_pc
ResumeEncoder->>ResumeDecoder: encode and decode rd_numb frame header
ResumeDecoder->>FrameRebuilder: provide RebuiltFrame with py_pc
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 |
… backxlat (jitcode-blackhole Slice 3' Phase B) `build_resumed_frames` now reads each frame's forward-carried `py_pc` from the resume data instead of deriving it via `backxlat_py_pc(jitcode_index, pc)` at decode. The Phase-A `PYRE_M73_PYPC_FWD_AUDIT` proved the two equal across the corpus (248/248, zero divergence), so the cutover preserves behavior. This removes the last resume-decode jitcode→python-pc reentry inverse: the `backxlat_py_pc` call site in `build_resumed_frames` is gone (the function keeps its bridge-trace-side callers). Drops the now-obsolete audit helper. check.py: dynasm 248/248 + cranelift 248/248. Assisted-by: Claude
3aaccdc to
13138a7
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: 3aaccdcb3e
ℹ️ 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".
| framestack: vec
When this fixture adds a py_pc word to the frame header, the encoded frame slot moves from items[8] to items[9], but the assertion a few lines below still decodes items[8] as a tagged slot. In this test items[8] is now the literal py_pc value 8, so untag(8) yields tag bits 0 instead of TAGBOX, leaving the required dynasm cargo test suite broken. Update the expected slot index/comment with the new three-word frame header.
AGENTS.md reference: AGENTS.md:L199-L201
Useful? React with 👍 / 👎.
| pub fn read_jitcode_pos_pc(&mut self) -> (i32, i32) { | ||
| let jitcode_pos = self.resumecodereader.next_item(); | ||
| let pc = self.resumecodereader.next_item(); | ||
| let _py_pc = self.resumecodereader.next_item(); |
There was a problem hiding this comment.
This unconditional third read is correct only if every rd_numb frame section emits py_pc, but the hand-built blackhole_from_resumedata_accepts_runtime_jitcode_without_canonical_pair fixture above still writes only jitcode_pos and pc for a zero-slot frame. In that scenario done_reading() is false after the two header words, read_jitcode_pos_pc() indexes past the end for _py_pc, and the test (and any similar runtime-only rd_numb fixture) panics instead of resuming. Add the py_pc word to those manual encodings.
AGENTS.md reference: AGENTS.md:L199-L201
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8d08ccd). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
Follow-on to the Slice 3' wire-format change (`jitcode_index, pc, py_pc` frame header): repair the tests that were still shaped for the two-word `(jitcode_index, pc)` header. - pyjitpl.rs: add the missing `py_pc` field to the `recorder::SnapshotFrame` built in `finish_trace_for_parity_preserves_captured_snapshots` (this was a `cargo test` compile break; the lib build did not cover it). - resume.rs: shift the hardcoded wire-offset assertions in five numbering tests by the inserted per-frame `py_pc` word and assert its value. - jitcode_dispatch/tests.rs: populate `forward_py_pc_marker_by_jit_pc` / `forward_py_pc_pred_by_jit_pc` on the guard-resume test JitCode so guard capture resolves the innermost frame's forward Python pc instead of aborting with `GuardResumeCoordinateUnavailable`. - history.rs: document that `capture_snapshot_for_last_guard` sets `py_pc == pc` because its native-driver / test callers have no CPython-bytecode coordinate split. check.py stays 248/248; majit-metainterp and pyre-jit test suites pass. Assisted-by: Claude
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Line 8039: Update the guard-failure frame reconstruction around the py_pc
assignment to store the forward-mapped Python PC from the codewriter marker data
rather than frame.pc, which is the JIT/resume PC. Preserve the existing -1 value
for synthetic frames and keep the separate JIT PC assignment unchanged.
In `@majit/majit-metainterp/tests/resume_parity.rs`:
- Around line 70-74: Add a direct encode/decode assertion for the negative
`py_pc` sentinel in the resume parity test around the `FrameInfo` fixture,
verifying that `-1` is preserved after serialization and deserialization. Keep
the existing positive `py_pc` coverage and assert the decoded frame value
explicitly.
In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 2916-2923: Update the top_py_pc fallback in the SnapshotFrame
construction to call request_trace_abort() when resolved exists but
resume_position_for_jitcode_pc(offset) cannot produce the forward py_pc twin
entry. Preserve the existing top_pc as u32 fallback only where appropriate, and
ensure missing py_pc mappings decline the trace rather than emitting an
unvalidated snapshot coordinate.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 9435-9443: Before merging the JIT change around the py_pc resume
logic, re-extract the corresponding Charon .ullbc files and run all eight
required benchmarks; reject the change if any benchmark regresses, and record
both validation results.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 13691-13696: In the marker-tier loop, compute skipped_py once
using skip_python_trivia_forward and reuse it for both
forward_py_pc_marker_by_jit_pc and the subsequent static_depth lookup for
depth_trivia. Mirror the single-computation pattern used by the predecessor-tier
loop.
🪄 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: 910bf44d-d589-45e1-bb87-f3d89f71b25d
📒 Files selected for processing (21)
majit/majit-backend/src/resume_value.rsmajit/majit-ir/src/resumedata.rsmajit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/history.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/recorder.rsmajit/majit-metainterp/src/resume.rsmajit/majit-metainterp/tests/jit_driver_runtime_parity.rsmajit/majit-metainterp/tests/resume_parity.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/pyjitcode.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-jit/src/call_jit.rs
| snapshot_frames.push(crate::recorder::SnapshotFrame { | ||
| jitcode_index, | ||
| pc: frame.pc as u32, | ||
| py_pc: frame.pc as u32, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Store the forward-mapped Python PC, not frame.pc.
frame.pc is already the JIT/resume PC stored on Line 8038. Copying it into py_pc collapses two distinct coordinate systems, so guard-failure reconstruction may resume the interpreter at the wrong Python bytecode location—especially for inlined frames. Use the frame value populated from codewriter marker data instead, preserving -1 for synthetic frames where required.
As per coding guidelines, the generated JIT must preserve interpreter semantics; a coordinate mismatch is a generation defect, not an acceptable porting difference.
🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs` at line 8039, Update the
guard-failure frame reconstruction around the py_pc assignment to store the
forward-mapped Python PC from the codewriter marker data rather than frame.pc,
which is the JIT/resume PC. Preserve the existing -1 value for synthetic frames
and keep the separate JIT PC assignment unchanged.
Source: Coding guidelines
| frames: vec![FrameInfo { | ||
| jitcode_index: 0, | ||
| pc: 77, | ||
| py_pc: -1, | ||
| slot_map: vec![ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert that the -1 sentinel round-trips.
The positive py_pc case is covered, but the sentinel cases do not assert the decoded value. Add a direct encode/decode assertion to catch signed serialization regressions.
Suggested assertion
let state = rd.reconstruct_state(&[7, 88]);
+ assert_eq!(rd.encode().decode().frames[0].py_pc, -1);🤖 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 `@majit/majit-metainterp/tests/resume_parity.rs` around lines 70 - 74, Add a
direct encode/decode assertion for the negative `py_pc` sentinel in the resume
parity test around the `FrameInfo` fixture, verifying that `-1` is preserved
after serialization and deserialization. Keep the existing positive `py_pc`
coverage and assert the decoded frame value explicitly.
| // Forward-carried Python resume pc, recorded at guard capture from the | ||
| // codewriter `(jitcode_pc, py_pc)` marker pair (no jitcode→py inverse at | ||
| // decode). py_pc=-1 is the no-snapshot sentinel (pc<0) → fall back to | ||
| // the vable next-instr. | ||
| let py_pc = if frame.py_pc >= 0 { | ||
| frame.py_pc as usize | ||
| } else { | ||
| vable_ni | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Complete the required JIT validation gates before merge.
The supplied verification covers compilation and backend checks, but not Charon .ullbc re-extraction or all eight benchmark runs. Verify both before merging and reject any benchmark regression.
As per coding guidelines, JIT changes require re-extracting the corresponding Charon files and running all eight benchmarks.
🤖 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 9435 - 9443, Before merging the JIT
change around the py_pc resume logic, re-extract the corresponding Charon .ullbc
files and run all eight required benchmarks; reject the change if any benchmark
regresses, and record both validation results.
Source: Coding guidelines
…rivia skip) Two follow-ups from the Slice 3' code review of the forward py_pc plumbing: - trace_opcode.rs: `build_framestack_snapshot` requested a trace abort when the resume JitCode pc could not be resolved, but the paired forward Python-pc lookup silently fell back to the raw pc. A resolved JitCode pc with no forward marker now requests the abort too, matching both the sibling `pc` fallback and the production `forward_snapshot_py_pc` hard-fail, instead of publishing a fallback Python pc. - codewriter.rs: drop the duplicate `skip_python_trivia_forward` call in the marker-tier loop; the value is already bound for the forward-py_pc marker push and is reused for the depth-trivia marker. check.py: dynasm 285/285 + cranelift 285/285. Assisted-by: Claude
…globals build_resumed_frames assigned inner (non-outermost) ResumedFrame.namespace the chain virtualizable's vable_ns. Assign it the callee's own w_code globals via w_code_get_w_globals instead, falling back to vable_ns when the callee code carries no globals. Mirrors recover_inline_callee_globals. Assisted-by: Claude
The six op-start-tier pred twins (forward_py_pc, depth_trivia, pcdep_trivia, const_ref_trivia, result_color_trivia, resume_marker) were built by pushing one entry per py_pc directly, leaving duplicate jitcode offsets whose binary-search winner depended on sort_unstable order. Build them from a BTreeMap<offset, py> (later py wins, matching by_off) so each offset yields one entry; drop the now redundant post-sorts. Duplicate py_pcs share skip_python_trivia_forward's target, so surviving values are unchanged (dynasm/cranelift 285/285). Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e623a80105
ℹ️ 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".
| /// Forward-carried Python instruction PC; `-1` is the no-snapshot | ||
| /// sentinel paired with `pc == -1`. | ||
| pub py_pc: i32, |
There was a problem hiding this comment.
When a multi-frame guard resumes at a JitCode PC whose forward Python PC differs from backxlat_py_pc (the trivia/after-residual/branch coordinates this field is meant to preserve), the bridge-reconstruction path still ignores RebuiltFrame.py_pc: reconstruct_inline_recipe derives py_pc with backxlat_py_pc(frame.jitcode_index, frame.pc) in pyre/pyre-jit-trace/src/state.rs:5927, and the bridge carrier still re-inverts root/recipe PCs in trace.rs:588, trace.rs:1048, and trace.rs:1237. That makes the new decoded field unused for inline bridge setup, so it can compute stack depth or pending-result slots from the wrong Python opcode and either drain otherwise valid bridges or rebuild an inlined callee at the wrong resume point. Please carry this py_pc through the carrier/recipe and use it in those consumers instead of the inverse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit/src/jit/codewriter.rs (1)
13797-13815: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDedup the after-residual op-start predecessors through
pred_by_off.The construction at lines 13797-13815 still pushes one entry per
first_jit_pc_by_py_pcentry before sorting, so duplicate JitCode offsets can remain in the three sibling vectors. Consumer readers perform binary search on these duplicate keys, so exact offset lookups can return an arbitrary earlier Python PC instead of the established “later py wins” semantics. Build/push the same deduped iteration here, as already done for forward/trivia and resume-marker op-start twins.🤖 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/jit/codewriter.rs` around lines 13797 - 13815, Update the op-start predecessor construction around after_residual_marker_pred_by_jit_pc, result_color_after_residual_pred_by_jit_pc, and depth_after_residual_pred_by_jit_pc to iterate through the established pred_by_off deduplicated mapping instead of directly enumerating first_jit_pc_by_py_pc. Preserve the existing later-Python-PC-wins semantics and ensure all three sibling vectors receive the same unique JitCode offsets before sorting.
🤖 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.
Outside diff comments:
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 13797-13815: Update the op-start predecessor construction around
after_residual_marker_pred_by_jit_pc,
result_color_after_residual_pred_by_jit_pc, and
depth_after_residual_pred_by_jit_pc to iterate through the established
pred_by_off deduplicated mapping instead of directly enumerating
first_jit_pc_by_py_pc. Preserve the existing later-Python-PC-wins semantics and
ensure all three sibling vectors receive the same unique JitCode offsets before
sorting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b0ac579a-6df7-4418-8570-2c91a41938cd
📒 Files selected for processing (3)
pyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
…ode-blackhole Slice 4 Phase A)
The inline-caller frame's after-residual-call resume python pc is computed at
resume time by inverting call_jit_pc then taking semantic_fallthrough_pc. Build
a codewriter twin after_residual_fallthrough_py_pc_{marker,pred}_by_jit_pc =
semantic_fallthrough_pc(RAW resolving py), keyed by JitCode offset alongside
depth_after_residual_* / result_color_after_residual_*, with accessor
after_residual_fallthrough_py_pc_for_jitcode_pc and a _populated predicate. No
consumer reads it yet; a PYRE_PCMAP_AFTERRESIDUAL_AUDIT assert at the
inline-caller seam certifies twin == inverted-then-fallthrough. Byte-identical
audit-off; dynasm/cranelift 285/285 both audit-off and audit-on.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dae04a213
ℹ️ 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 callee_py_pc = | ||
| forward_snapshot_py_pc(callee_jitcode_index as u32, callee_jitcode_pc as u32)?; |
There was a problem hiding this comment.
When a multi-frame inline guard is captured after a residual call in the callee (after_residual_call == true), callee_jitcode_pc names the post-call resume marker, but this new generic forward lookup records the containing CALL's Python pc instead of the semantic fallthrough pc. build_resumed_frames uses the innermost frame's carried py_pc for multi-frame resumes, so deoptimizing such a guard can re-enter at the CALL and repeat an already-executed residual side effect. Fresh evidence beyond the earlier inline-resume comment is that this newly added encoding line ignores the after_residual_call parameter even though the diagnostic path just above explicitly advances via semantic_fallthrough_pc.
Useful? React with 👍 / 👎.
…twin (jitcode-blackhole Slice 4 Phase B) resolve_parent_resume_py_pc's CallFallthrough arm and the self-recursive CALL_ASSEMBLER vstack seed computed the inline-caller frame's after-residual resume python pc by inverting call_jit_pc then taking semantic_fallthrough_pc. Read the forward after_residual_fallthrough_py_pc twin instead; the inversion survives only for the empty-twin class (populated code, no Python map) and as the PYRE_PCMAP_AFTERRESIDUAL_AUDIT oracle. The marker-miss depth raw() fallbacks stay Python-keyed (already covered by depth_after_residual). Byte-identical audit-off; dynasm/cranelift 285/285 both audit-off and audit-on. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d08ccde8e
ℹ️ 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".
| .push((pos, result_color_at_pc.get(ft_rc).copied())); | ||
| depth_after_residual_pred_by_jit_pc | ||
| .push((pos, static_depth.get(ft_rc).copied())); | ||
| after_residual_fallthrough_py_pc_pred_by_jit_pc.push((pos, ft_rc as u32)); |
There was a problem hiding this comment.
When several Python PCs share the same first JitCode offset (the duplicate-offset case handled just above with pred_by_off), this new table still appends one entry per Python PC and then relies on sort_unstable plus a binary search in after_residual_fallthrough_py_pc_for_jitcode_pc. For an inline CALL at such an offset, lookup can pick a neighboring trivia/EXTENDED_ARG PC's raw semantic_fallthrough instead of the CALL's fallthrough, so parent-frame/vstack resume can re-enter at the wrong Python PC. Build this table from the deduped offset map, with the same deterministic “later py wins” policy, before publishing it.
Useful? React with 👍 / 👎.
Every flag below was default-on with an unused `=0` opt-out. Delete the gate machinery and make the enabled behavior unconditional; the disabled arms and their helper code are removed as dead. Removed env flags (collapsed to always-on): PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED. exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`: native backends run the exception-edge bridge unconditionally; the wasm guest's abort-replay exception class (#727) is still open, so it stays off there. check.py: drop the --no-fbw-inline-multiframe option and its PYRE_FBW_INLINE_MULTIFRAME=0 export. Assisted-by: Claude
Every flag below was default-on with an unused `=0` opt-out. Delete the gate machinery and make the enabled behavior unconditional; the disabled arms and their helper code are removed as dead. Removed env flags (collapsed to always-on): PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED. exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`: native backends run the exception-edge bridge unconditionally; the wasm guest's abort-replay exception class (#727) is still open, so it stays off there. check.py: drop the --no-fbw-inline-multiframe option and its PYRE_FBW_INLINE_MULTIFRAME=0 export. Assisted-by: Claude
Every flag below was default-on with an unused `=0` opt-out. Delete the gate machinery and make the enabled behavior unconditional; the disabled arms and their helper code are removed as dead. Removed env flags (collapsed to always-on): PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED. exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`: native backends run the exception-edge bridge unconditionally; the wasm guest's abort-replay exception class (#727) is still open, so it stays off there. check.py: drop the --no-fbw-inline-multiframe option and its PYRE_FBW_INLINE_MULTIFRAME=0 export. Assisted-by: Claude
Every flag below was default-on with an unused `=0` opt-out. Delete the gate machinery and make the enabled behavior unconditional; the disabled arms and their helper code are removed as dead. Removed env flags (collapsed to always-on): PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED. exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`: native backends run the exception-edge bridge unconditionally; the wasm guest's abort-replay exception class (#727) is still open, so it stays off there. check.py: drop the --no-fbw-inline-multiframe option and its PYRE_FBW_INLINE_MULTIFRAME=0 export. Assisted-by: Claude
Every flag below was default-on with an unused `=0` opt-out. Delete the gate machinery and make the enabled behavior unconditional; the disabled arms and their helper code are removed as dead. Removed env flags (collapsed to always-on): PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED. exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`: native backends run the exception-edge bridge unconditionally; the wasm guest's abort-replay exception class (#727) is still open, so it stays off there. check.py: drop the --no-fbw-inline-multiframe option and its PYRE_FBW_INLINE_MULTIFRAME=0 export. Assisted-by: Claude
…rollout-flag cleanup (#757) * jit: exc-edge bridge fixes (forward catch lookup, dynasm exception-cell ops, stale exception seed) - walker: add find_catch_for_exc_resume — the blackhole handle_exception_in_frame forward case (catch_exception directly after the resume -live-, blackhole.py:396) tried before the backward scan; the exc-edge routing in dispatch_via_miframe now uses it. - dynasm aarch64/x86: implement SaveExcClass (load pos_exception, assembler.py:1817-1818), SaveException (shared emit_store_and_reset_exception, assembler.py:1820-1821) and RestoreException (assembler.py:1845-1850). The previous SaveExcClass/SaveException bodies returned 0 and RestoreException had no emit arm. - walker: seed_standing_exception_for_walk reads BH_LAST_EXC_VALUE before the preseeded-sym early return, so the exception published from the current guard failure overwrites exception state a previous walk left on the persistent sym (_prepare_exception_resumption grabs from this failure's deadframe, pyjitpl.py:3125-3126). A preseeded sym is kept only when no fresh publish exists. All three changes are exercised only with PYRE_EXC_EDGE_BRIDGE set. check.py 293/293 on dynasm and cranelift. Assisted-by: Claude * jit: guard the exception flavor at exception-guard bridge entry A loop trace recorded through a raising iteration carries that iteration's GUARD_EXCEPTION(class). No-raise iterations chronically fail it WITHOUT a pending exception, so a bridge is compiled for the no-exception continuation; a second exception class then enters the same bridge WITH a pending exception and the recorded continuation runs on the NULL raised-call result (SIGSEGV in compiled code, both backends, default mode). - walker: an exception-guard bridge walk with no standing exception now records GUARD_NO_EXCEPTION at bridge entry (_prepare_exception_resumption null arm + prepare_resume_from_failure, pyjitpl.py:3152-3171), so the pending-exception flavor deopts to the blackhole at entry. - cranelift: attached-bridge in-code dispatch now also runs for must_save_exception guards, before the exception staging in emit_guard_exit — entering the bridge with the exception cells intact, as dynasm's patched guard jump does (patch_jump_for_descr). The previous host-loop re-entry consumed the exception before invoking the bridge, so the entry flavor guard could not see it. - call_jit: decline bridge compilation from GUARD_NOT_FORCED failures — "Failures of a GUARD_NOT_FORCED are never compiled, but always just blackholed" (ResumeGuardForcedDescr.handle_fail, compile.py:950-953). - bench: add synth/exc_mixed_classes_bridge_flavor covering the two-exception-class shape. check.py 293/293 on dynasm and cranelift. Assisted-by: Claude * jit: carry the bridge-entry flavor guard's resume coordinate verbatim The routed/null bridge-entry flavor-guard captures fed the walk-entry position — already a post-call resume coordinate — through the after-residual capture path, whose op-START-keyed twins advanced it a second time, onto the physically-following except-handler block. The entry guard's own bridge then resumed inside the handler, and its other-flavor decode failed the exc-edge catch lookup (ExcEdgeCrossFrameReturnUnsupported retry loop). Add GuardCaptureScope::carried_resume_jit_pc: the entry captures carry position verbatim as the guard's resume word, take the resume py from the forward twin at that word, and skip the op-START-keyed depth twins (they read the key opcode's depth, over-publishing valuestackdepth so the resume read garbage slots as Refs). Assisted-by: Claude * jit: clear the standing-exception seed on a no-exception bridge failure seed_standing_exception_for_walk kept a preseeded sym exception when the published cell was empty. For an exception-guard bridge the publish is the deadframe-grab authority, so an empty cell now clears the seed (_prepare_exception_resumption null arm, pyjitpl.py:3152-3154). Previously a no-exception failure of an exception guard walked a stale exception's handler as the no-exception continuation, and the per-flavor bridge chain recompiled the same handler indefinitely instead of converging. Assisted-by: Claude * gc: write-barrier JIT-side virtualizable frame stores and exc child walks The guard-failure vable sync (write_boxes_to_heap), the walk-end escape flushes, and the MidBody abort commit stored decoded/boxed refs into a frame's locals_cells_stack_w raw. The values can be nursery-young while the frame/array are old-gen and the virtualizable runs detached from the walked frame chain, so no minor re-traced the items; a traceback-reachable frame then fed the stale nursery address to major marking, panicking in incremental_mark_step (invalid type_id; reproducible with MAJIT_GC_STRESS=1 PYRE_EXC_EDGE_BRIDGE=1 on a reraise loop). - majit-metainterp write_field / write_array_item: arm the object/array in the remembered set after every Ref store (virtualizable.py:101-113 write_boxes stores run under the translated write barrier upstream). - pyre-jit-trace flush/commit paths and execute_assembler entry: re-arm the frame + array via frame_array_write_barrier. - Exception root walkers (walk_jit_exc_value, walk_active_sym_exc_roots, PyError::walk_gc_refs): forward the non-moving carrier's raw child slots so young tracebacks/args parked across a minor stay valid; add the missing BH_LAST_EXC_VALUE walker. Verified: stress battery clean on both backends; adversarial exc battery matches CPython flag-on and flag-off; check.py 298/298 dynasm and cranelift. Assisted-by: Claude * jit: enable the exception-edge bridge by default on native backends PYRE_EXC_EDGE_BRIDGE becomes opt-out (=0 disables) on native targets; the wasm guest keeps the opt-in gate (no env plumbing to switch it back off, and its abort-replay exception class is still open). Native jitstats baselines regenerated: every bench's top-level print loop previously hit the pending-exception decline and now compiles (+1 loop, +1 guard failure); fib_recursive additionally converges two GuardNoException bridges (bridges 1->3, guard_failures 1->407, absorbed in warmup). loops_aborted / internal_compile_panics stay 0 everywhere. wasm baselines unchanged. A/B (same binary, env toggle, alternating x3): exc_mixed_classes_ bridge_flavor 0.415s -> 0.166s; handler_reraise_second_exc ~6% faster; no bench regressed. check.py 298/298 on dynasm and cranelift with the default on; adversarial exc battery matches CPython both with the default and with =0. Assisted-by: Claude * jit: remove the served-their-purpose default-on FBW rollout flags Every flag below was default-on with an unused `=0` opt-out. Delete the gate machinery and make the enabled behavior unconditional; the disabled arms and their helper code are removed as dead. Removed env flags (collapsed to always-on): PYRE_FBW_INLINE, _INLINE_MULTIFRAME, _NSVABLE_MULTIFRAME, _REC_MULTIFRAME, _BRIDGE_REC_INLINE, _REC_MUTUAL_CUTOVER, _REC_CA, _FORITER_INLINE, _LOOP_CALLEE_CA, _RAISE, _BUILTIN_FOLD, _LOADATTR_FOLD, _STOREATTR_FOLD, _LOADMETHOD_FOLD, _LOADGLOBAL_FOLD, _LOADNAME_FOLD, _STORENAME_FOLD, _DELETE_FAST, _INLINE_NSFOLD, _STACK_LIVEREG, _CALL_ASSEMBLER, _NO_REPLAY_EXIT, _NESTED_RESID_ABORT, _ABORT_FLUSH, _BRANCH_FLUSH, _END_FLUSH, _BRIDGE_STAMP, _BRIDGE_LOCAL_SEED. exc_edge_bridge_enabled() becomes `cfg!(not(target_arch = "wasm32"))`: native backends run the exception-edge bridge unconditionally; the wasm guest's abort-replay exception class (#727) is still open, so it stays off there. check.py: drop the --no-fbw-inline-multiframe option and its PYRE_FBW_INLINE_MULTIFRAME=0 export. Assisted-by: Claude * gc: re-arm the frame-array write barrier after every flush store The walk-end / abort flush functions box each Int/Float slot (an allocation that can trigger a minor collection) one at a time into the detached frame array. The array is forwarded by a minor collection only while it is in the remembered set, and each minor consumes that entry, so a single barrier after the whole loop left a window: a nursery Ref stored in one iteration could be dropped by a minor collection triggered by the next iteration's boxing before the array was re-armed, leaving a stale pointer in the resumed frame. Re-arm the barrier after every store (and before the first allocation that follows a pre-loop nursery store) in flush_walk_end_state_to_frame_inner, flush_walk_end_state_at_outer_call, write_back_outer_locals, and flush_walk_end_state_after_outer_call. Assisted-by: Claude * jit(gc): only strip a bridge's save/restore-exception prefix when its result is unused remove_bridge_exception stripped a leading SaveExcClass + SaveException + RestoreException prefix unconditionally (rewrite.py:988), leaving its `XXX should check if the boxes are used later` deferred. A routed exception-guard handler bridge records that same prefix but keeps the SaveException result as last_exc_value for handler code (`except E as e`), so an unconditional strip drops an operand a later op still references. Scan the ops after the prefix for a use of the RestoreException class/value operands (args and failargs) and strip only when neither is reused. Add regression tests for the strip-when-unused and keep-when-reused cases. Assisted-by: Claude * jit: wrap the FBW end-flush loop-close call to satisfy rustfmt The rebase renamed `flush_walk_end_state_to_frame` to `flush_walk_loop_end_state_to_frame` in the end-flush block, pushing the `else if` condition past the 100-column limit; rustfmt moves the opening brace to its own line. Assisted-by: Claude * jit(cranelift): drop the post-staging attached-bridge dispatch for exception guards emit_guard_exit dispatched the attached bridge twice for a can_have_bridge + must_save_exception guard: once before the _store_and_reset_exception staging and once after it. The pre-staging dispatch was widened to cover must_save guards in 618871e, so its guard set now supersets the post-staging block's. The post-staging dispatch enters the bridge with the exception globals already cleared, so the bridge entry flavor guard (prepare_resume_from_failure) reads no pending exception; a bridge installed between the two probes would take that wrong-flavor path. dynasm's patched guard jump enters the bridge before the failure-recovery stub stages jf_guard_exc (patch_jump_for_descr, x86/assembler.py:987), matching the pre-staging dispatch. Remove the redundant second dispatch. check.py 304/304 on cranelift. Assisted-by: Claude
1 participant
jitcode-blackhole epic — Slice 3': eliminate the guard-fail resume-decode backxlat inverse
Continues #709. At a guard failure,
build_resumed_framesreconstructed each resumed frame'sPython pc by computing it backward via
backxlat_py_pc(jitcode_index, jitcode_pc)— the lastjitcode→python-pc inverse on the guard-fail resume-decode path. This slice carries the Python pc
forward as resume data instead — matching PyPy
resume.pyconsume_boxes, where the frame pc isrestored forward as data and never computed backward — then deletes the backward call site.
Phase A — forward
py_pc+ audit twin (byte-identical)Adds a per-frame
py_pcword to the resume-data frame header, sourced forward from the codewriter(jitcode_pc, py_pc)marker pair at guard capture (not projected through the inverse at encode, whichwould merely relocate the backxlat). Threads it through
recorder::SnapshotFrame,resume::SnapshotFrame,FrameInfoBuilder/push_frame, andRebuiltFrame; writes it in all threeframe-header encoders (
number()+ the two compactrd_numbencoders) and consumes it in bothdecoders (
rebuild_from_numberingand the blackholeread_jitcode_pos_pc). Wire order isjitcode_index, pc, py_pc, [boxes]uniformly; synthetic single frames use the-1sentinel.build_resumed_framesstill derives the value viabackxlat_py_pc; underPYRE_M73_PYPC_FWD_AUDITitasserts the forward word equals the backxlat result for every frame. Off by default, so production
decode is byte-identical. Also removes the dead
ResumedFrame.rd_numb_pcfield (no readers).Phase B — cutover
build_resumed_framesnow reads the forwardframe.py_pcinstead of callingbackxlat_py_pc. ThePhase-A audit proved the two equal across the corpus, so the cutover preserves behavior. This removes
the last guard-fail resume-decode jitcode→python-pc reentry inverse call site — the
backxlat_py_pcfunction itself stays for its bridge-trace-side callers. Drops the now-obsolete audit helper.
Verification
cargo check/cargo test --no-run(--features dynasm): clean.python3 pyre/check.py --backend dynasm,cranelift= 248/248 each with the audit off(byte-identical) and
PYRE_M73_PYPC_FWD_AUDIT=1(zero divergence).python3 pyre/check.py --backend dynasm,cranelift= 248/248 each.Summary by CodeRabbit
PYRE_NO_JITswitch to skip tracing/JIT compilation.