Repository navigation
jit: record the save/restore-exception pair on a no-exception bridge entry - #772
Conversation
…entry `_prepare_exception_resumption` records SAVE_EXC_CLASS + SAVE_EXCEPTION for the exception-guard descr flavor whether or not the deadframe carried an exception (exc_class = 0 and a null value when it did not), and `prepare_resume_from_failure` records RESTORE_EXCEPTION just before the entry guard (pyjitpl.py). The pair keeps the optimizer from deleting that guard: resume data can place a removable operation ahead of it, and `optimize_GUARD_NO_EXCEPTION` (rewrite.py, pure.py) drops a GUARD_NO_EXCEPTION whose predecessor was removed. Upstream pins this with test_guard_no_exception_incorrectly_removed_from_bridge (issue #2132). The bridge sub-walk emitted the trio only on the routed pending-exception arm; the no-exception arm recorded a bare GuardNoException. Both `optimize_GUARD_NO_EXCEPTION` ports exist here (rewrite.rs on `ctx.last_op_removed`, pure.rs on `last_emitted_was_removed`), and `seed_execution_context_for_walk` records a heap-CSE-eligible GetfieldGcR immediately ahead of that guard, so the removal shape is reachable. Record the same trio on the no-exception arm. The guard survives today — no bench changes behaviour with or without this commit, and the added bench passes either way — so this closes the removal window rather than fixing an observed miscompile. RestoreException is never folded by any pass, so it holds `last_op_removed` clear; `remove_bridge_exception` strips the trio once it is consecutive and its results unused, which is the case on this arm. Also drops stale upstream line-number suffixes from the surrounding comment. check.py dynasm 308/308 + cranelift 308/308 on the parent commit; with this change the new bench and exc_mixed_classes_bridge_flavor are bit-identical to pypy on both backends, and no jitstats baseline drifts. Assisted-by: Claude
|
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 (2)
WalkthroughAdds a standalone benchmark for repeated exception-guard bridge execution and expands documentation for the corresponding ChangesException guard handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit cf5763a). 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
|
What
_prepare_exception_resumptionrecordsSAVE_EXC_CLASS+SAVE_EXCEPTIONfor the exception-guard descr flavor whether or not the deadframe carried an exception (exc_class = 0and a null value when it did not), andprepare_resume_from_failurerecordsRESTORE_EXCEPTIONjust before the entry guard (pyjitpl.py).That pair is not decoration — it is what keeps the optimizer from deleting the entry guard. Resume data can place a removable operation ahead of the guard, and
optimize_GUARD_NO_EXCEPTION(rewrite.py,pure.py) drops aGUARD_NO_EXCEPTIONwhose predecessor was removed. Upstream pins the hazard withtest_guard_no_exception_incorrectly_removed_from_bridge(issue #2132).The bridge sub-walk emitted the trio only on the routed pending-exception arm; the no-exception arm recorded a bare
GuardNoException. This records the same trio on that arm.Why it matters here
The removal shape is structurally present in this tree, not hypothetical:
optimize_GUARD_NO_EXCEPTIONports exist —rewrite.rskeys onctx.last_op_removed,pure.rsonlast_emitted_was_removed;seed_execution_context_for_walkrecords a heap-CSE-eligibleGetfieldGcRimmediately ahead of the entry guard.If that guard is removed, the bridge is entered with a pending exception at the same source guard and runs the recorded no-exception continuation on a NULL raised-call result.
Scope — this closes a window, it does not fix an observed miscompile
Stated plainly: the guard survives today. A control build with this change reverted produces identical output on the added bench, so nothing observable changes.
RestoreExceptionis never folded by any pass, so it holdslast_op_removedclear;remove_bridge_exceptionstrips the trio again once it is consecutive with its results unused, which is the case on this arm — so the protection costs nothing at runtime.Also drops stale upstream line-number suffixes from the surrounding comment.
Verification
check.pydynasm 308/308 + cranelift 308/308 on the parent commit.exc_bridge_entry_guard_not_removed→(28000, 4000)on dynasm, cranelift, no-JIT, pypy3 and cpython;MAJIT_STATSconfirms it really compiles a bridge (bridges_compiled=1), so it is not a vacuous shape.exc_mixed_classes_bridge_flavorunchanged at2399980000, matching pypy3 on both backends.A
check.pyrun taken while this machine was at load average ~118 (several sibling worktrees building concurrently) showed three chronic boundary perf gate-sitters failing —raise_catch,const_arg_call_resume,residual_raise_except_resume. Correctness was 306/306 non-perf and no jitstats drifted, so those are load artifacts rather than an effect of this change.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation