jit: drop the dead exception-bridge prologue and the unread walk-sym exception getters - #1240
Conversation
…exception getters `MetaInterp::emit_exception_bridge_prologue` had no callers. The live exception-edge bridge emits the sequence itself in `bridge_subwalk`, through `trace_ctx.save_exc_class()` / `save_exception()` / `restore_exception()`, which record0 the two SAVEs as `pyjitpl.py:3148-3149` does. The dead copy passed the class and value as operands instead, and pointed at `optimizeopt/rewrite.rs` for `remove_bridge_exception`, which lives in `majit-gc/src/rewrite.rs` (rewrite.py:988-1001). Its one piece of live information — that RPython splits the sequence across `_prepare_exception_resumption` and `prepare_resume_from_failure` because resume-data replay runs between the SAVEs and the RESTORE, and that pyre may collapse them only while it emits nothing in that gap — moves onto the live emitter. `WalkSym::current_exc_value` / `current_exc_box` had no callers either. The setters and the fields stay: `walk_active_sym_exc_roots` reads `sym.current_exc_value` directly as a GC carrier. In `must_compile_with_values`, the per-typetag match had the same body in all four arms. compile.py:761-771 does convert — `cast_ptr_to_int` for TY_REF and `longlong.gethash_fast` for TY_FLOAT — but `gethash_fast` is `longlong2float.float2longlong` on a 64-bit host (codewriter/longlong.py:28), so all three tags hash the raw word `fail_values` already holds. Replaced with that note plus compile.py:772-773's `assert 0, typetag`. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 21 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 (3)
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 8803872). 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)
4. Structural adaptationsNone. |
![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: 8803872e0d
ℹ️ 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".
| // host (codewriter/longlong.py:28) — the double's raw bit pattern. | ||
| // `fail_values` already holds every slot as that raw word, so all | ||
| // three tags hash the stored value unchanged. | ||
| let intval: i64 = fail_values.get(index as usize).copied().unwrap_or(0); |
There was a problem hiding this comment.
On wasm32 and other 32-bit targets, RPython's rpython/jit/codewriter/longlong.py:44-45 defines gethash_fast as intmask(xll - (xll >> 32)), and the Rust port already implements that target-specific behavior in majit-translate/src/codewriter/longlong.rs:64-76. Treating the raw word as the hash ignores its upper 32 bits because JitCounter::_get_index truncates to u32; distinct values such as 1.0 and 2.0 therefore feed the same per-value counter and can spuriously reach trace eagerness from their combined failures. Keep the raw-word shortcut only on 64-bit targets and apply the folded gethash_fast calculation for TY_FLOAT on 32-bit targets.
AGENTS.md reference: AGENTS.md:L288-L290
Useful? React with 👍 / 👎.
`arith_int_bool` reads `bridges_compiled` 11 and `guard_failures` 2307 where the file recorded 10 and 2211; `short_circuit_value_kept_stack` reads 11 and 2201 where it recorded 12 and 2510. Both are main's, not this branch's: the CI run on `3b403691725` reports the same two fixtures with the same numbers, and they do not move across `#1238`/`#1240` or with the environment byte length varied from 0 to 512. The ratio row on `short_circuit_value_kept_stack` is a separate inherited red and is not addressed here. Assisted-by: Claude
1 participant
Follow-up to #1232. This round re-read the bridge/retrace surface against the
vendored RPython sources looking for more of what #1222 and #1232 fixed. It
found no wrong-code defect. What it did find is dead code that had drifted
away from the live path it documents, so that is what this PR removes.
emit_exception_bridge_prologuewas dead, and no longer matched the live pathMetaInterp::emit_exception_bridge_prologuehad no callers. The liveexception-edge bridge builds the sequence itself in
bridge_subwalk, viatrace_ctx.save_exc_class()/save_exception()/restore_exception(), whichrecord0 the two SAVEs exactly as
pyjitpl.py:3148-3149does. The dead copypassed the class and value as operands instead — a shape the backends'
genop_save_exc_class(result-only, no arglocs) would not accept — and citedoptimizeopt/rewrite.rsforremove_bridge_exception, which lives inmajit-gc/src/rewrite.rs(rewrite.py:988-1001).It did carry one thing worth keeping: that RPython splits the sequence across
_prepare_exception_resumptionandprepare_resume_from_failurebecauseresume-data replay runs between the SAVEs and the RESTORE, and that collapsing
them into one block is sound only while pyre emits nothing in that gap. That
note moves onto the live emitter, where the next reader will be standing.
Two
WalkSymaccessors had no callerscurrent_exc_value()/current_exc_box()are gone. The setters and the fieldsstay —
walk_active_sym_exc_rootsreadssym.current_exc_valuedirectly as aGC carrier, so the state is live even though these two getters never were.
A per-typetag match with four identical arms
must_compile_with_valuesdecoded the GUARD_VALUE typetag and then returned thesame expression for TY_INT, TY_REF, TY_FLOAT and the fallthrough.
compile.py:761-771 really does convert —
cast_ptr_to_intfor TY_REF,longlong.gethash_fastfor TY_FLOAT — butgethash_fastislonglong2float.float2longlongon a 64-bit host (codewriter/longlong.py:28),i.e. the double's raw bit pattern, which is what
fail_valuesalready holds forevery slot. So no arm needs a conversion. Replaced with that reasoning plus
compile.py:772-773's
assert 0, typetag, so the next reader does not have tore-derive it from the RPython side.
What was checked and came back faithful
Recording this so the next pass starts further along rather than re-walking it:
bridgeopt.pyserialize/deserialize on both sides of the class bitfield and theheap/loopinvariant sections;
serialize_optheap's three skip conditions(
get_descr_index() == -1,_lazy_set,parent_descr.is_object()) and thearray side's
index >= 2**15; theST_BUSY_FLAG/ST_TYPE_MASKstatus machineand
make_a_counter_per_valueacross all four backends;store_hash(pyreassigns per compiled fail-descr layout rather than at optimizer-emit time, and
both bridge paths are covered);
_copy_resume_data_fromand theResumeGuardCopiedDescrprevchase, including the blackhole resume;propagate_original_jitcell_tokenagainst the retrace path, wherecombined_ops = partial.ops + body_opsputs the prior front's LABEL inside thetrace so the token set and the LABEL set coincide; all seven
optimize_bridgeexits, the
vs-survives-InvalidLoopfallthrough on the retrace-limit retry,and the
retraced_countread and write both landing oncell_token_key; andrecord_loop_or_bridge's quasi-immutable dependency publication on the bridgepath.
Verification
pyre/check.py, darwin arm64, all three backends:Neither touched crate is in the LLBC extraction set, and the change is dead-code
removal plus comments, so no jitstats row can move — the green run is a
regression check, not a measurement.
— authored by Claude