Repository navigation
jit: name the handler's live-in in every -live- an exception table covers - #993
Conversation
…vers `compute_liveness` unions a handler's alive set into the markers before its `catch_exception`: the op carries `TLabel(L)`, so the backward pass folds the label's set in (liveness.py:19-23). The coordinate an after-residual-call guard records sits PAST that `catch_exception`, so its own marker describes the no-exception arm alone. In `bench/synth/list_length_hint_validate.py`'s `classify_extend` the call's trailing marker names ref `[3, 9, 10, 11]`, the recorded resume marker names `[9, 10]`, and the catch target needs `[3, 9, 11]`. `route_exc_edge` re-points the walk from that coordinate into the handler with a bank seeded from the recorded set, so the handler's first frame write-back read the local `r` (color 3) unbound: 20 x `RegisterReadUnbound`, `loops_aborted 14 -> 34` against the tracked baseline. Upstream has no equivalent gap. `finishframe_exception` moves the pc of the SAME MIFrame, so the handler reads the bank the raise left behind; a pyre bridge rebuilds its bank from one coordinate's resume data, so that coordinate has to name what the handler reads. Union the handler PC's marker Ref colors into the marker of every py_pc `lookup_exceptiontable` covers. Keyed on the exception table rather than on the SSA `catch_exception`: the runtime's recorded resume py is not `semantic_fallthrough_pc(call_pc)`, so keying it that way augments a different marker and measures as a no-op. Re-keying `pos_for_pc[fallthrough]` to the call's trailing marker instead fixes the same fixture but moves the walk START before the `catch_exception`, and a bridge resuming with the exception already active then walks onto it: `exception_reraise_tb_depth_jitstress` goes 900/0/7/1798 to 893/0/30/0 with `CatchExceptionWithActiveException`. check.py: dynasm 368/369, cranelift 368/369, wasm 363/365. Every remaining failure reproduces unchanged with this commit reverted (`exception_args_virtual` on all three, `pickle_terminal_raise_resume` on wasm), as does the one `cargo test --all --no-default-features --features dynasm` failure (`jit_fnaddr::tests::registered_paths_sharing_an_address_are_alias_spellings`). Of the 354 tracked jit-stats baselines per backend only `list_length_hint_validate` moves, back to its recorded 2/4/14/828. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 25 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 (1)
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: 0d0dbaa9da
ℹ️ 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".
| // `handler py -> live-in`, memoized: a try body of N PCs shares one handler. | ||
| let mut handler_refs: std::collections::HashMap<usize, std::collections::BTreeSet<u16>> = | ||
| std::collections::HashMap::new(); |
There was a problem hiding this comment.
This introduces a HashMap for handler_py, even though the key is a dense instruction index bounded by live_markers.len() and the surrounding comment explicitly says there is no upstream equivalent whose dict shape is being preserved. Use indexed storage such as Vec<Option<BTreeSet<u16>>> instead; the repository requires proving the corresponding RPython/PyPy container before adding a map and specifically prefers a Vec for small or dense key spaces.
AGENTS.md reference: AGENTS.md:L115-L124
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0d0dbaa). 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)None. 4. Structural adaptations
|
What
bench/synth/list_length_hint_validate.pyregressedloops_aborted 14 -> 34against its tracked jit-stats baseline. The +20 is exactly 20 x
RegisterReadUnbound { pc: 456, reg: 3, bank: "r" }inclassify_extend.Why
compute_livenessunions a handler's alive set into the markers before itscatch_exception— the op carriesTLabel(L), so the backward pass folds thelabel's set in (
liveness.py:19-23). The coordinate an after-residual-callguard records sits PAST that
catch_exception, so its own marker describes theno-exception arm alone. Measured from the runtime liveness map of that jitcode:
live.ref_-live-, directly beforecatch_exception[3, 9, 10, 11]catch_exception/L 420[9, 10]route_exc_edgere-points the walk to[3, 9, 11]route_exc_edgere-points the walk 373 -> 420 with a bank seeded only from373's set, so the handler's first frame write-back reads the local
r(color 3) unbound and aborts the walk.
Upstream has no equivalent gap:
finishframe_exceptionmoves the pc of theSAME MIFrame, so the handler reads the bank the raise left behind. A pyre
bridge rebuilds its bank from one coordinate's resume data, so that coordinate
has to name what the handler reads.
How
Union the handler PC's marker Ref colors into the marker of every py_pc
lookup_exceptiontablecovers. Widening a-live-set is the conservativedirection — the snapshot carries boxes the no-exception arm does not consume,
which costs a slot and never mis-restores.
Keyed on the exception table rather than on the SSA
catch_exception: theruntime's recorded resume py is not
semantic_fallthrough_pc(call_pc), sokeying it that way augments a different marker and measures as a no-op.
Rejected alternative
Re-keying
pos_for_pc[fallthrough]to the call's trailing marker fixes the samefixture exactly, but moves the walk START before the
catch_exception; a bridgeresuming with the exception already active then walks onto it.
exception_reraise_tb_depth_jitstressgoes900/0/7/1798to893/0/30/0withCatchExceptionWithActiveException. Widening the marker's live set is safe;moving the coordinate is not.
Verification
Measured on base
c0891858087:exception_args_virtualexception_args_virtualexception_args_virtual,pickle_terminal_raise_resumecargo test --all --no-default-features --features dynasmregistered_paths_sharing_an_address_are_alias_spellingsEach remaining failure reproduces unchanged with this commit reverted. Of the
354 tracked jit-stats baselines per backend only
list_length_hint_validatemoves, back to its recorded
2/4/14/828.Re-verification against the current base (
6fbef5f7c7c, #982) is running andthis section will be updated with those numbers.
— authored by Claude