jit: retire #371 pcdep regalloc + exception-CFG adaptations - #800
Conversation
|
Warning Review limit reached
Next review available in: 29 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 (8)
WalkthroughChangesJIT exception and liveness pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GraphBuilder
participant CodeWriter
participant GraphFlattener
participant ResumeDispatcher
participant BlackholeInterpreter
GraphBuilder->>CodeWriter: classify and lower can-raise operation
CodeWriter->>GraphFlattener: emit raising op and -live- marker
GraphFlattener->>ResumeDispatcher: emit catch_exception edge
ResumeDispatcher->>BlackholeInterpreter: scan resume layout
BlackholeInterpreter->>ResumeDispatcher: return adjacent handler or propagate
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 |
![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: 91358e9c04
ℹ️ 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".
| .rposition(|op| { | ||
| op.offset == py_pc as i64 | ||
| && super::flatten::graph_op_can_raise(op) | ||
| }) | ||
| .map(|raising_pos| raising_pos + 2) |
There was a problem hiding this comment.
When a protected bytecode expands to multiple can-raise graph operations, this rposition attaches the exception edge only after the last one. For example, Instruction::LoadSpecial records load_special followed by load_special_self, and graph_op_can_raise classifies both as raising; if the first residual raises, its trailing -live- is followed by the second call rather than catch_exception, so find_catch_for_exc_resume cannot find the in-frame handler and propagates an exception that the surrounding try should catch. Each can-raise operation at the PC needs its own guessexception-style block split and exception edge.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2e2f72d). 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)None identified within the audited changed-file scope beyond the structural adaptations below. 4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d9c756ce
ℹ️ 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 cfg_variable_pairs = super::regalloc::filter_coalesce_pairs_by_interference( | ||
| &graph, | ||
| Kind::Ref, | ||
| &cfg_variable_pairs, | ||
| &[], | ||
| ); |
There was a problem hiding this comment.
When a CFG coalesce chain connects variables from different semantic frame slots whose lifetimes never overlap—specifically, an inlined callee's operand-stack temporary and an outer-frame merge input—the forced -live- arguments create no dependency edge, so this filter accepts the merge. The removed build_slot_disjoint_interference path handled exactly this non-co-live case; without it, one color spans two slots and can be reported live at a resume point where regs_r[color] is OpRef::NONE, causing guard-resume failures or collect_outer_active_boxes panics. Retain a semantic-slot constraint in addition to ordinary graph liveness.
Useful? React with 👍 / 👎.
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c257ad965
ℹ️ 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".
| // Successor continuation: identity split — same | ||
| // Variables, no freshening (the UNPACK_SEQUENCE / | ||
| // FOR_ITER split precedent). The block's own |
There was a problem hiding this comment.
For every protected opcode whose post-raise operations are moved into this successor, this explicitly reuses the predecessor's Variable identities as the new block's input arguments and operands. The referenced rpython/translator/unsimplify.py:55-74 instead copies each predecessor-defined variable and rewrites the moved operations, preserving the per-block SSA definition boundary; skipping that refactor lets regalloc and subsequent graph passes conflate the predecessor lifetime with the successor definition. Port the actual split_block variable-map behavior rather than using an identity split.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
… B1) Close a caught can-raise op's walker block at the op's trailing -live- (flowcontext.py guessexception): record_graph_op appends a structural trailing -live- for can-raise graph ops (graph_op_can_raise), and the generic per-PC catch-attach site splits the current block after that -live- instead of emitting a placeholder pair, moving the post-op vable stores into the successor block with split_block varmap threading. flatten.rs canraise lowering now reads the graph tail directly: - block_can_raise = last op is -live- (flatten.py:206-217 shape), replacing the emitted-stream scan - the hoisted_tail rewrite, pc_first_insn_pos shift, and the release adjacency assert are deleted - Block::raising_op skips trailing -live- markers The jitcode layout changes from [call, live, stores, catch] to [call, live, catch, live(block entry), stores]. Adjust find_catch_before_resume_live (blackhole.rs and the walker mirror) to hop over exactly one -live- (the successor's block-entry marker) and accept only a catch_exception directly behind it. check.py dynasm/cranelift: 301 passed; the 2 remaining failures (const_arg_call_resume perf, getframe_force_cancel_journal output) reproduce on the committed-HEAD baseline binary. Assisted-by: Claude
…se_value (issue #371 tail B2) A covered explicit raise now records a `raise` graph op as the block's last operation before attaching the catch-landing exception edge, so the standard flattener serializes `raise <value>` from the block body and the single-exit canraise arm emits the byte-adjacent `catch_exception` from graph structure alone. Deleted: - Link::explicit_raise_value (flow.rs) and its copy/rename plumbing - carry_explicit_raise_value_on_catch_stack + its DELETE_FAST call site and unit test - the flatten single-exit arm's out-of-band raised-value read; the arm now keys on block.canraise() with a debug assert that the raising op is the `raise` The two raise-terminated-block detectors (canraise_pending, block_closed_by_terminator) read Block::raising_op() == "raise" instead of scanning link fields. check.py dynasm/cranelift: 301 passed each; the 2 remaining failures (const_arg_call_resume perf, getframe_force_cancel_journal output) reproduce on the committed-HEAD baseline binary. Assisted-by: Claude
Emit each per-PC \`-live-\` as a graph SpaceOperation carrying the frame-live Ref Variables as force-alive args (liveness.py:8-12), so RegAllocator::make_dependencies models CPython frame-slot liveness structurally: co-live frame slots interfere, the chordal coloring keeps them on distinct colors, and the coalesce filter's has_edge guard (regalloc.py:105) rejects frame-lifetime-overlap merges. Production Ref allocation is now one make_dependencies -> coalesce_variables -> find_node_coloring pass. Deleted: pcdep_canonical_slot, build_slot_disjoint_interference, build_value_parent, build_colive_interference, perform_register_allocation_with_pairs_and_interference, perform_register_allocation_all_kinds_with_pairs_and_interference, add_interference_pin_ids, the extra_interference parameter of filter_coalesce_pairs_by_interference, and the second Ref allocation pass with its interference wiring. validate_pcdep_color_map now builds its value partition from the coalesce pairs directly. Serialization keeps the marker argless (compute_liveness derives the runtime resume window from actual uses); the forced args exist only on the graph for the allocator. Follow-up adjustments this exposed: - flatten: pc_first_insn_pos skips \`-live-\` insns so a PC whose only insn is its own resume marker stays stack-only for derive_pc_live_indices_from_sparse's re-keys. - codewriter/eval: merge_entry_by_green covers only greens the body emits ops at-or-after; a truncated body (mid-function abort_permanent) no longer hands a later loop header a carry-forward marker whose walk re-executes already-run bytecode on abort flush. compile_and_run_once declines such greens instead of asserting. - resume_snapshot: compute_inline_caller_frame falls back from the result_color trivia twin to the after-residual twin when the CALL's trailing marker is not the fallthrough PC's block-head marker. Validation: cargo test --features dynasm green; check.py dynasm 311 passed / cranelift 310 passed with only pre-existing failures (getframe_force_cancel_journal, cranelift const_arg_call_resume perf); cranelift A/B on fib_recursive / raise_catch_loop / fannkuch / nbody perf-neutral vs 4bfa40b3028. Assisted-by: Claude
depth_at_py_pc rebuilt the truncated/saturated u16 depth table on every call. LiveVars is immutable after construction and cached per code pointer by liveness_for, and the resume/handback paths query the table repeatedly per code object, so store it in a OnceLock built on first use and return a borrow. 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: 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/src/jit/flatten.rs`:
- Around line 1759-1766: Promote the invariant checks from debug-only to
release-enforced assertions: replace the debug_assert! around the single-exit
canraise validation in pyre/pyre-jit/src/jit/flatten.rs:1759-1766, and the
corresponding can-raise/trailing-live validation in
pyre/pyre-jit/src/jit/codewriter.rs:12264-12289, with assert!. Both consumers
must fail loudly when the recorded raise termination or split point is invalid.
🪄 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 Plus
Run ID: 3d3569c5-4e99-4a4e-92e4-3e2d614b78a9
📒 Files selected for processing (8)
majit/majit-metainterp/src/blackhole.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/liveness.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-jit/src/jit/flatten.rspyre/pyre-jit/src/jit/flow.rspyre/pyre-jit/src/jit/regalloc.rs
| if block.borrow().canraise() { | ||
| debug_assert!( | ||
| block | ||
| .borrow() | ||
| .raising_op() | ||
| .is_some_and(|op| op.opname == "raise"), | ||
| "single-exit canraise block must be raise-terminated" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Both consumers of the "can-raise op → trailing -live-" invariant validate it with debug_assert!, which no-ops in release. The invariant is established once, in record_graph_op (codewriter.rs), but consumed at two structurally critical sites that each drive exception-CFG shaping; if the invariant is ever violated by a future walker regression, both sites silently produce corrupted/incorrect exception dispatch or block-splitting with no diagnostic, instead of failing loud like the many other assert!/panic! invariants introduced in this same rewrite.
pyre/pyre-jit/src/jit/flatten.rs#L1759-L1766: promote toassert!so a single-exit canraise block that isn't actually raise-terminated fails loud instead of silently emitting exception dispatch bytes.pyre/pyre-jit/src/jit/codewriter.rs#L12264-L12289: promote toassert!so a mis-detected split point fails loud instead of silently moving the wrong post-raise operations into the successor block.
📍 Affects 2 files
pyre/pyre-jit/src/jit/flatten.rs#L1759-L1766(this comment)pyre/pyre-jit/src/jit/codewriter.rs#L12264-L12289
🤖 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/flatten.rs` around lines 1759 - 1766, Promote the
invariant checks from debug-only to release-enforced assertions: replace the
debug_assert! around the single-exit canraise validation in
pyre/pyre-jit/src/jit/flatten.rs:1759-1766, and the corresponding
can-raise/trailing-live validation in
pyre/pyre-jit/src/jit/codewriter.rs:12264-12289, with assert!. Both consumers
must fail loudly when the recorded raise termination or split point is invalid.
1 participant
Closes #371.
Retires the two remaining codewriter/regalloc adaptations #371 tracks, plus a self-contained liveness cleanup. All four commits are structural — no behavior change; guard/blackhole resume stays byte-exact against the no-JIT oracle.
Regalloc tail (Tail A)
Per-PC graph
-live-operations carry the frame-liveRefvariables as force-alive args (liveness.py:8-12shape), somake_dependenciesmodels CPython frame-slot liveness structurally and the productionRefallocation follows one upstream-shaped dependency → coalesce → color pass. Deletespcdep_canonical_slot,build_slot_disjoint_interference,build_value_parent,build_colive_interference, and the pair/interference allocation wrappers used only by the splice adaptation. The-live-marker is serialized argless (the graph args are regalloc-side only;compute_livenessderives the runtime window from actual uses), so bridge sub-walk reconstruction never sources a register whose value only lives in the virtualizable.Exception-CFG tail (B1 + B2)
record_graph_opappends a structural-live-at each caught can-raise op (flowcontext.py:130-156 guessexception), so the ordinaryflatten.py::insert_exitsport emits the correct adjacent-live-/catch_exceptionsequence from graph structure. Replaces the emitted-streamblock_can_raisepredicate with theflatten.py:206-217graph check (block.operations.last()is-live-) and deletes thehoisted_tailreconstruction + thepc_first_insn_posshift.raisein the graph/exception links consumed by the standard flattener and deletesLink::explicit_raise_value.Liveness cleanup
LiveVars::depth_at_py_pcmemoizes its per-PCu16depth table in aOnceLockinstead of rebuilding a freshVecon every call (LiveVarsis immutable and cached per code pointer byliveness_for; the resume/handback paths query it repeatedly).#371 Done criteria
Regalloc tail — single upstream dependency/coalesce/color pass; the four pcdep helpers + splice-only wrappers deleted; runtime guard/bridge resume unambiguous for every live local/stack slot. ✓
Exception-CFG tail — structural flowgraph block boundaries at caught can-raise ops matching
flowcontext.py; explicit raise reaches the standard flattener withoutLink::explicit_raise_value; the emitted-streamblock_can_raisescan andhoisted_tailreconstruction deleted;catch_exceptionadjacency follows from graph structure rather than a repair. ✓Validation
cargo check/cargo test --features dynasmgreen; fullcheck.pyon dynasm + Cranelift (CI); no Cranelift regression onfib_recursive/raise_catch/fannkuch/nbody; byte-exact guard/blackhole resume vs the no-JIT oracle.— description written by Claude
Summary by CodeRabbit
Bug Fixes
Performance