majit: retarget bare assertion-raise blocks instead of widening the pass - #1208
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe pull request consolidates field-mint census counters, adds offset-source classification, updates runtime statistics and determinism reporting, and strengthens assertion-error graph pruning with validation and tests. ChangesField-mint census and determinism
Assertion-error graph pruning
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The change narrows assertion-edge removal to intermediate blocks whose operations are removable, preserving branches that perform non-removable work; focused tests and generated-output comparisons remain unchanged, so no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 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: 27ad14a2cf
ℹ️ 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 indirect = exit.target != exceptblock; | ||
| let assertion_exit = if indirect { | ||
| let target = graph.block(exit.target); |
There was a problem hiding this comment.
This still broadens simplify.remove_assertion_errors by accepting an edge whose target is an intermediate block rather than graph.exceptblock; checking that the block's operations satisfy CanRemove makes deletion safer, but it does not reproduce upstream's control-flow shape or direct-only condition. Collapse or retarget the raise block in the corresponding simplification phase, then keep this pass's predicate literal, rather than retaining a second implementation with different eligibility rules.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 53df3623ac1, taking the second half of the suggestion literally.
remove_assertion_errors is back to upstream's exit.target is graph.exceptblock with no indirection. The retarget now happens in the simplification sequence that runs before it: retarget_assert_raise_blocks points an edge whose target exists only to raise the implicit AssertionError straight at exceptblock, gated on the block having a single exit carrying that raise and every operation satisfying can_remove_op (CanRemove, simplify.py:411-423). The bypassed block keeps its exits and falls out as unreachable, as eliminate_empty_blocks leaves the blocks it rewires past.
I first tried the phase that already exists for this — collapse_panic_message_chains, whose doc says it collapses panic chains "to the bare raise so remove_assertion_errors prunes the branch as it does for a direct implicit raise" — by widening its panic_block_is_pure_message to the same predicate. That does not work, and the reason is ordering: the phase runs after simplify_lowered_graph, so remove_assertion_errors has already had its chance. Instrumenting the rejection path confirmed neither do_warn_explicit nor w_member_get_direct_kind ever presents an assert-raising block to it; the 24 blocks it does reject across the corpus are all genuine side-effecting calls (handle_alloc_error, os::exit, core::intrinsics::unreachable).
Generated code is unchanged against the previous revision at the same base and corpus: identical jitcodes_index.bin, identical jitcode name list, 2645 jitcodes both, zero jitcodes differing in bytecode, register counts, constant-pool sizes, result types or start points, and the same single unrepairable constants_r entry.
— commented by Claude
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: 53df3623ac
ℹ️ 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 mut raise_args: std::collections::HashMap<BlockId, Vec<LinkArg>> = | ||
| std::collections::HashMap::new(); |
There was a problem hiding this comment.
This new side table is keyed by BlockId, whose dense index already addresses graph.blocks, but there is no corresponding map in upstream simplify.remove_assertion_errors and no justification for departing from its storage shape. Use indexed Vec/Option storage or structure the retargeting without a side table; the repository specifically requires proving upstream semantics before introducing a HashMap and directs dense identity-keyed lookups toward a vector-like container.
AGENTS.md reference: AGENTS.md:L115-L124
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit c17fa7e). 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
|
#1206 taught `remove_assertion_errors` to follow a one-block indirection, which broadened `simplify.remove_assertion_errors` past its literal `exit.target is graph.exceptblock` and, because the guard looked only at the intermediate block's exit count, deleted whatever operations that block held. The comment justifying the widening cited `join_blocks`, which folds a target only into an unconditional predecessor (`simplify.py:283-286`) — not into the branch this exit leaves. `retarget_assert_raise_blocks` runs before the pass and points an edge whose target exists only to raise the implicit `AssertionError` straight at `exceptblock`, which is where the flow space puts it upstream. A block qualifies when it has a single exit carrying that raise and every operation it holds satisfies `can_remove_op` — `CanRemove` (`simplify.py:411-423`), the predicate the dead-op pass would apply to those operations once the raise is gone. The bypassed block keeps its exits and falls out as unreachable, as `eliminate_empty_blocks` leaves the blocks it rewires past. `remove_assertion_errors` goes back to upstream's literal predicate. Two other placements were tried and measured against the corpus: requiring the intermediate block to be empty, and widening the front's `panic_block_is_pure_message` so `collapse_panic_message_chains` would normalise these raises. Both left all three unrepairable `constants_r` entries and the `jitcodes.bin` / `jit_metadata.json` in-process determinism failure in place — the second because the shape never reaches that phase, which runs after `simplify_lowered_graph`. Generated code is unchanged against #1206's guard at this base and corpus: identical `jitcodes_index.bin`, identical jitcode name list, 2645 jitcodes both, zero jitcodes differing in bytecode, register counts, constant-pool sizes, result types or start points, one unrepairable `constants_r` entry (`_unpackiterable_unknown_length` `0x8`) on both, and 13/13 reproducible outputs identical in-process. Assisted-by: Claude
1 participant
Follow-up to #1206, addressing the Codex parity review's §1 finding on that PR and the P1 on the first revision of this one.
The finding
#1206 taught
remove_assertion_errorsto follow a one-block indirection. Codex objected twice, and both times correctly:The fix
remove_assertion_errorsgoes back to upstream's literal predicate. A newretarget_assert_raise_blocksruns immediately before it insimplify_lowered_graphand points an edge whose target exists only to raise the implicitAssertionErrorstraight atexceptblock— where the flow space puts it upstream (flowcontext.py_implicit_), which is why the upstream predicate can be literal in the first place. The bypassed block keeps its exits and falls out as unreachable, the wayeliminate_empty_blocks(simplify.py:52-69) leaves the blocks it rewires past.A block qualifies when it has a single exit carrying that raise and every operation it holds satisfies
can_remove_op—CanRemove(simplify.py:411-423), the predicate the dead-op pass would apply to those operations once the raise is gone.can_remove_oprather thanis_pure_op: the question is what may be deleted, andinline.rs:1130-1135already records that upstream keepsLLOp.is_pure()andCanRemovedistinct for that reason.Two other placements were tried and rejected by measurement
constants_rentries and the in-process determinism failure.panic_block_is_pure_messagesocollapse_panic_message_chainsnormalises these raises — the phase whose own doc says it collapses panic chains "to the bare raise soremove_assertion_errorsprunes the branch as it does for a direct implicit raise". Also disarms the change. Instrumenting the rejection path showed why: that phase runs aftersimplify_lowered_graph, and neitherdo_warn_explicitnorw_member_get_direct_kindever presents an assert-raising block to it. Its 24 rejections across the corpus are all genuine side-effecting calls (handle_alloc_error,os::exit,core::intrinsics::unreachable), so widening its predicate buys nothing here.The normalisation therefore belongs in the pass sequence that runs before
remove_assertion_errors, which is where it now sits.Generated code is unchanged
Against #1206's guard at the same base and corpus (
af392b9a):all_jitcodesjitcodes_index.binconstants_r_unpackiterable_unknown_length0x8)jitcodes.binanddescrs.binare inHOST_ADDRESSED_OUTPUTSand are not comparable across processes, so the comparison is on the address-free fields.Tests
majit-translatelib suite green (3193 passed). One new test pins both directions of the retarget predicate: a raise block holding only aConstIntis retargeted and then pruned; one holding agetslice— outsideCanRemove— keeps its edge and survives the pass.Note on main's current CI
pyre/check.pyis red on ubuntu and windows and the CPython gate is red, all of which predate #1206: main atb0f34c0af3efailed the same three jobs with byte-identical rows —wasm synth/exception_traceback_loop_formsguard_failures 810 -> 811andcranelift synth/str_fstring659 -> 658, cranelift 1 failed/424 passed, wasm 1 failed/417 passed.b0f34c0af3ere-recordedexception_traceback_loop_formsfor dynasm and cranelift (811) and left wasm at 810. Not touched here.— authored by Claude