jit: four bridge/retrace parity gaps, the x86 GUARD_VALUE counter stamp, and the f32 decay multiply - #1222
Merged
Conversation
…helper does counter.py:278-279 hands `decay_by_mult` to `pypy__decay_jit_counters`, whose C body does `float f = (float)f1` once and then multiplies each entry in single precision. pyre widened the entry to f64, multiplied by the f64 multiplier and narrowed the product, which rounds twice and disagrees whenever the exact product lands near an f32 tie. `would_tick_fire` narrows the same way, so the predicate still agrees with the `tick` that drains the same generations. Reported by the Codex parity review on #1207 as a pre-existing mismatch. It predates the decay drain but that change makes it compound: the multiply now runs once per elapsed interval instead of saturating at one. Assisted-by: Claude
regalloc.py:496-499 consider_guard_value calls `op.getdescr().make_a_counter_per_value(op, loc)` while laying every GUARD_VALUE out. pyre does this on aarch64 dynasm, cranelift and wasm; the x86 emitter had it when `dynasm: x86/aarch64 split` created the file and `backend+metainterp: unify descr->JCT identity via meta_descr back-pointer` deleted it along with 254 other lines, leaving the aarch64 twin untouched. Without the stamp the descr's status keeps the plain per-guard hash `store_hash` writes (compile.py:826-829 only fires while `status == 0`), so `must_compile` takes compile.py:745's common-case arm instead of compile.py:753-781's `addr(descr) * 777767777 + intval * 1442968193`. Every failure of a given GUARD_VALUE then ticks one bucket regardless of which value failed, so a polymorphic guard reaches trace_eagerness on aggregate failures and compiles a bridge specialised to a value that does not recur. The block is the aarch64 one verbatim, including the fail-arg index: pyre's `must_compile_with_values` reads the value back out of `fail_values`, where upstream passes a register index. The x86 module is gated on `target_arch = "x86_64"`, so an aarch64 host never compiles it and the local gate cannot reach this path. Verified with `cargo check --target x86_64-apple-darwin -p majit-backend-dynasm`; the runtime effect is only observable on the linux and windows CI legs, where it will move jit-stats counters that currently record the unbucketed behaviour. Assisted-by: Claude
`VirtualStateInfo::VArray::items` and `VArrayStruct::element_fields` held `Rc<VirtualStateInfoNode>` with no way to express upstream's `None` fieldstate. `export_state` reached its unreachable-panic for an unwritten slot's `OpRef::NONE` operand; a `cfg!(test)` arm fabricated an `Unknown(Type::Int)` leaf instead, so test builds numbered a slot that a release build could not export at all. Both slot vectors are now `Option<Rc<VirtualStateInfoNode>>`, filled by a named `create_state_or_none` mirroring `VirtualStateConstructor.create_state_or_none` (virtualstate.py:707-710). `_generate_guards`, `_enum`, `enum_forced_boxes`, the numbering and counting walks, the GC walk, position resets, deep clone, state import and label-argument import all skip absent entries. The VArray and VArrayStruct guard arms take upstream's asymmetry verbatim (virtualstate.py:257-260, :316-320): expected absent continues, expected present against incoming absent raises. The `cfg!(test)` fabrication is gone; the export panic remains and now distinguishes the absent-operand sentinel from a ref with no recorded type. Assisted-by: Claude
`handle_fail_resume_guard` (majit-backend-dynasm) runs the bridge hook and the blackhole hook back to back over one host `Vec<i64>` that is not a GC root, where compile.py:701-717 is an exclusive if/else and never reaches `resume_in_blackhole` after a successful attach. `jit_ca_handle_guard_failure` roots its own copy across the bridge hook but published it through `CA_WALK_RESUME_DEADFRAME` only on the `!compiled` arm, so the successful-attach leg left the blackhole reading the caller's un-rooted buffer. Publish on both arms. `CA_WALK_ADOPTED_FRAME` was stamped from inside the walk closure, before the bridge was optimised and compiled. Stamp it at the return sites instead, from a fresh `FrameRoot::frame` read, the way the four `CA_WALK_RESUME_FRAME` stamps already do; the four resume stamps and the new adopted stamp share one helper. `jit_blackhole_resume_from_guard` compares that cell against the deadframe's slot 0, so both sides now name the frame's current address. Assisted-by: Claude
`fixup_target_tokens` runs at the end of both `assemble_loop` and `assemble_bridge` upstream (x86/assembler.py:612 and :706), and pyre's x86, aarch64 and cranelift backends all run their publish step on the bridge path. wasm stamped `set_label_block_id` and called `publish_label_target` only from `compile_loop`, while nothing declines a LABEL-bearing bridge — `compile_retrace` (compile.py:341-393) reaches the backend through `send_bridge_to_backend`, so a retrace is a bridge with its own LABEL, and its label never entered `LABEL_TARGETS`. The stamp + publish block moves into `stamp_and_publish_label_targets`, called by `compile_loop` with its own handle and frame and by `compile_bridge` with `bridge_slot` and `source_frame` past every decline. The existing peeled / non-peeled split needs no new gate: a retrace closing onto its own target token is peeled and publishes at key ordinal + 1, while a `jump_to_preamble` retrace is not peeled and its label stays unpublished. `CompiledWasmLoop` gains `bridge_owned_label_targets`, and `Drop` retracts those entries under the same "the registry still names this handle" guard it applies to the loop's own labels; a bridge module already lives as long as its source loop. Counter 21 no longer fires for a bridge with no LABEL, which has nothing to publish and nothing withheld. Assisted-by: Claude
compile.py:496-502 `forget_optimization_info` is the first statement of both `send_loop_to_backend` (compile.py:506-507) and `send_bridge_to_backend` (compile.py:571-572); pyre had no port, so the optimizer's output `Rc<Op>`s reached the backend and were stored verbatim as `CompiledTrace.ops` with their `forwarded` slots still populated, pinning that trace's `PtrInfo` graph for the artifact's lifetime. Port it over `OpRc` and `InputArg` and call it at the five loop/bridge backend boundaries, immediately ahead of the profiler scope. The `reset_values` arm stays unported: neither send-to-backend call site passes it. `UnrollOptimizer::check_no_forwarding` existed with no caller; wire it as a `debug_assert!` on the bridge leg, where unroll.py:188 asserts it. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 25 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 (9)
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 45f9e5c). 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)
4. Structural adaptations
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four unported pieces of the bridge/retrace path, plus two counter fixes carried
over from #1207's review.
Bridge / retrace
jit: represent an unwritten virtual-array slot as absent fieldstateVArray::itemsandVArrayStruct::element_fieldsheld a bareRc<VirtualStateInfoNode>, so there was no way to say what upstream says withNone. An unwritten slot'sOpRef::NONEreachedexport_state'sunreachable-panic — except under
cfg!(test), where an arm fabricated anUnknown(Type::Int)leaf, so test builds numbered a slot a release build couldnot export at all. Both vectors become
Option<...>, filled through a namedcreate_state_or_none(virtualstate.py:707-710), and the VArray / VArrayStructguard arms take upstream's asymmetry verbatim (virtualstate.py:257-260,
:316-320): expected absent continues, expected present against incoming absent
raises.
jit: hand the CA blackhole leg the deadframe copy this hook rootedcompile.py:701-717 is an exclusive if/else, but pyre's
handle_fail_resume_guardruns the bridge hook and the blackhole hook back toback over one host
Vec<i64>that is not a GC root.jit_ca_handle_guard_failurerooted its own copy but published it throughCA_WALK_RESUME_DEADFRAMEonly on the!compiledarm, leaving the successfulattach reading the caller's un-rooted buffer. Publish on both arms.
CA_WALK_ADOPTED_FRAMEwas also stamped from inside the walk closure, beforethe bridge is optimised and compiled — work that allocates. It now stamps at the
return sites from a fresh
FrameRoot::frameread, sharing a helper with thefour
CA_WALK_RESUME_FRAMEstamps.wasm: publish the LABEL target tokens a bridge definesfixup_target_tokensruns at the end of bothassemble_loopandassemble_bridgeupstream (x86/assembler.py:612 and :706), and pyre's x86,aarch64 and cranelift backends all run their publish step on the bridge path.
wasm ran it only from
compile_loop. Nothing declines a LABEL-bearing bridge:compile_retrace(compile.py:341-393) reaches the backend throughsend_bridge_to_backend, so a retrace is a bridge with its own LABEL, and itslabel never entered
LABEL_TARGETS. The existing peeled / non-peeled splitneeds no new gate — a retrace closing onto its own target token is peeled and
publishes, while a
jump_to_preambleretrace is not peeled and correctly staysunpublished.
CompiledWasmLoopgainsbridge_owned_label_targetsand retractsthem in
Dropunder the same registry guard the loop's own labels use.jit: clear optimizer forwarding before handing a trace to the backendforget_optimization_info(compile.py:496-502) is the first statement of bothsend_loop_to_backendandsend_bridge_to_backend; pyre had no port, so theoptimizer's output ops reached the backend and were stored verbatim as
CompiledTrace.opswith theirforwardedslots still populated, pinning thattrace's
PtrInfograph for the artifact's lifetime. Ported overOpRcandInputArgand called at the five backend boundaries. Thereset_valuesarmstays unported — neither call site passes it.
check_no_forwardingexisted withno caller; it is now a
debug_assert!on the bridge leg where unroll.py:188asserts it.
Counters (follow-ups to #1207's review)
dynasm(x86): restore the GUARD_VALUE per-value counter stampregalloc.py:496-499 calls
make_a_counter_per_valuewhile laying out everyGUARD_VALUE. aarch64 dynasm, cranelift and wasm all do; the x86 emitter had it
and lost it in an unrelated 254-line deletion. Without the stamp the descr keeps
store_hash's plain per-guard hash, somust_compiletakes compile.py:745'scommon-case arm instead of the per-value bucketing at compile.py:753-781 — every
failure of a polymorphic GUARD_VALUE ticks one bucket and it compiles a bridge
specialised to a value that does not recur. x86-only code, so the local darwin
gate cannot reach it; verified with
cargo check --target x86_64-apple-darwin -p majit-backend-dynasm. It will movejit-stats counters on the linux and windows legs.
jit: narrow the decay multiplier to f32 before multiplying, as the C helper doespypy__decay_jit_countersdoesfloat f = (float)f1once and multiplies insingle precision. pyre widened each entry to f64, multiplied by the f64
multiplier and narrowed the product — two roundings, which disagree when the
exact product lands near an f32 tie.
would_tick_firenarrows the same way sothe predicate still agrees with the
tickdraining the same generations.Verification
pyre/check.pyon darwin arm64, all three backends: dynasm 427/427.Two categories of pre-existing red remain, neither from this branch:
synth/str_fstringcranelift jitstats,657 -> 658. The committed per-OSbaselines for this fixture disagree with what each host observes, on main
itself; the same +1 gap is measurable on this base with these four commits
reverted. jit: count eval-breaker back-edge poll failures separately #1194 shifted the observation and the baseline together by one, so
the gap is unchanged. Being re-recorded separately.
sibling worktrees building), so these are not measurements: this run failed
cranelift fib_recursive(5s timeout) andwasm fannkuch(3.2x), while anearlier run on a quieter machine passed both and instead failed
global_quasiimmut_invalidationandshort_circuit_value_kept_stack— tworows main's own ubuntu leg also fails, along with
raise_catch, which thisbranch passes. CI is the measurement that counts here.
The x86 GUARD_VALUE stamp cannot be exercised by a darwin gate at all; it is
compile-checked against
x86_64-apple-darwinand will show up on the linux andwindows legs.
— authored by Claude