Repository navigation
jit: plain-root-bridge self-recursive inline lift + consume_boxes-parity bridge resume - #704
Conversation
|
Warning Review limit reached
Next review available in: 58 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 Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 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: e5b0df092b
ℹ️ 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".
| if !(fbw_bridge_rec_inline_enabled() && safe_root_bridge) { | ||
| return Ok(None); |
There was a problem hiding this comment.
For a root bridge with a builtin-int BinaryOp callee, this condition now skips the old Ok(None) decline before we know whether the callee will actually get a multi-frame parent/seed. The seed block below is best-effort for strict callees: if it hits cases like ncells != 0, missing portal reds, or an unavailable caller frame, it just leaves callee_frame_seeded == false/no parent and still inlines via the single-frame collapse. That re-admits the bridge hazard described above this block—guards after the inlined arithmetic residual can resume with the callee frame/operand stack not represented—whereas the previous code kept these bridge calls residual.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ae96d7d). 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 adaptations
|
…RIDGE_REC_INLINE The bridge-trace decline that keeps an exact-integer arithmetic callee a single residual call now still fires except on a plain root bridge walk (no carrier resume, not an inline sub-walk, empty framestack, live root portal) when PYRE_FBW_BRIDGE_REC_INLINE is set; there the call falls through to the self-recursive unroll gate and the multiframe seed as a primary walk does. Default off; with the flag off the walk is byte-identical to before. Assisted-by: Claude
Flip the plain-root-bridge self-recursive inline lift (added behind the env gate in 05db80c7173) from default off to default on. `=0`/`false` opts back out. Update the fbw_state and inline_call doc comments to match. Assisted-by: Claude
Assisted-by: Claude
…boxes totality reconstruct_inline_recipe classified a paused portal callee with two enumerated shapes: the canonical [frame, ec] reds-only form and the reduced [frame] in-a-call form. A middle frame paused mid-expression keeps operands live on the stack beside the reds (the left result across the second CALL of f(a) + g(b)), a wider live-ref set both shapes reject; it fell to the color==semantic arm, which declines on the unmappable portal frame red and collapses the carrier to a zero-recipe plain bridge. Replace the two shapes with one predicate: accept any paused portal callee whose every live non-red color maps to a semantic locals_cells_stack_w slot through the per-pc pcdep inversion. The portal arm reconstructs each slot semantic-ordered from the frame virtual's array, so the reds are recovered through the frame and each live operand is decoded from the pause-time flush; the mandatory-operand NONE check still declines an uncovered slot. This subsumes both prior shapes and narrows the gap to consume_boxes (resume.py:1042-1057), which fills every live register with no classifier. Record P2Recipe census keys at the admission and decline points. Assisted-by: Claude
Assisted-by: Claude
e5b0df0 to
e6e88a5
Compare
youknowone
changed the title
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae96d7d7b4
ℹ️ 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".
| action @ (TraceAction::RecursiveCallAssembler { .. } | TraceAction::Abort) => { | ||
| action @ (TraceAction::RecursiveCallAssembler { .. } | ||
| | TraceAction::Abort | ||
| | TraceAction::Decline) => { |
There was a problem hiding this comment.
When a bridge full-body walk hits one of the new FBW_BRIDGE_DECLINED paths, returning TraceAction::Decline now enters this same abort arm, so it still executes self.meta.abort_trace_live(false) below. That helper calls warm_state.abort_tracing, which increments the owning cell's abort count/logs an abort and can mark the whole green key DONT_TRACE_HERE after repeated declined bridge guards, even though the guard itself is already recorded in declined_bridge_guards and the new enum contract says this path should not charge a trace abort. Workloads with several structurally declined guards on one compiled loop can therefore suppress future tracing/reattachment for the whole key instead of only blackholing those guards.
Useful? React with 👍 / 👎.
…d-residual decline A PYRE_FBW_BRIDGE_REC_INLINE root-bridge inline of a self-recursive callee reaches its own recursive CALL as a nested residual during the callee body sub-walk, which fbw_abort_nested_unjournaled_residual declined on the self-recursive hazard arm — an abort storm that folds the guard bridge back to residual. The native CALL_ASSEMBLER self-recursion fold already exempts that decline via SELFREC_CA_FOLD_ACTIVE; set the same flag around the admitted root-bridge inline's body sub-walk so the recursive residual runs concretely at the pre-execute site. Native only (cfg): the wasm always-portal path type-confuses the self-recursive inline, so it keeps the residual-fallback decline. bridge_recursion_overflow: bridges_compiled 0->1, loops_aborted 6->1, guard_failures 1307->360, byte-parity; fib_recursive unchanged. dynasm + cranelift 294/294. Assisted-by: Claude
…he nested-residual hazard decline The depth-1 self-recursive inline lift inlines a root-bridge self-recursive callee one level; that callee's own recursive residual then trips the code_is_self_recursive arm of fbw_abort_nested_unjournaled_residual and aborts the whole bridge. Add PYRE_FBW_BRIDGE_REC_RESID (default on), a thread-local BRIDGE_REC_LIFT_TOP_INLINE marker armed by an RAII guard only across the #704-admitted callee sub-walk, and a pre_execute site discriminant on fbw_abort_nested_unjournaled_residual, so that single residual reaches the executor instead of aborting. fbw_inline_callee_hazardous now returns which hazard arm fired; the exemption applies only to SelfRecursiveStatic, only at the pre-execute gate (residual_call.rs), only when the framestack holds exactly the depth-1 admitted top frame, and only on native (wasm keeps declining). The loop-bearing, duplicate-frame, and executor-declined arms keep aborting. Assisted-by: Claude
…-inline depth to 7 (#749) * jit: route list-append fold decline through a journaled residual The LIST_APPEND opcode fold (ListAppendValue) aborted the trace with UnfoldableListAppendResidualUnsupported on any decline. The dominant decline is the realloc boundary: orthodox_list_append_recognize returns None via !w_list_can_append_without_realloc because the fold lowers only the in-place fast store, while w_list_append's full-block Vec::push else-leg is an un-lowered allocating helper. That aborted the resize side-exit's bridge. Fall through to the generic jit_list_append residual instead of aborting, and journal the pre-append length in try_execute_residual_call_via_executor (list_append_journal, mirroring inplace_list_journal) so an aborting walk's rollback rewinds the eager append and the replay re-applies it exactly once. Native (dynasm/cranelift): listcomp_hot loops_aborted 1->0 / bridges_compiled 0->1; comprehension_object_append_hot 126->2; nested_list_comprehension_hot 44->2. check.py dynasm 248/248, cranelift 248/248. wasm keeps the safeguard abort (cfg!(target_arch = "wasm32")): its codegen miscompiles the append-residual bridge (element drop), the same hazard wasm_unboxed_append_fold_declined guards the fold against. check.py wasm 247/247. Assisted-by: Claude * jit: mirror boxed const operand for value LOAD_CONST A value `LOAD_CONST` (large int / float) writes its result to the walker's unboxed int/float bank, so `write_ref_reg` never stamps `vstack_last_ref` and the Ref-only operand mirror left the stack slot a NONE hole. `stack_sync` omitted the slot and a single-frame bridge resume rebuilt it NULL; when the operand was a following CALL argument the callee parameter reconstructed unbound (UnboundLocalError on a literal `f(1000)` in a hot loop that grows a list). Materialize the boxed constant in the ResultToTos reconcile so the mirror carries it, matching MIFrame.registers_r holding a Const box. Add synth/const_arg_call_resume regression bench. Assisted-by: Claude * jit: exempt the #704 root-bridge self-recursive inline from the nested-residual decline A PYRE_FBW_BRIDGE_REC_INLINE root-bridge inline of a self-recursive callee reaches its own recursive CALL as a nested residual during the callee body sub-walk, which fbw_abort_nested_unjournaled_residual declined on the self-recursive hazard arm — an abort storm that folds the guard bridge back to residual. The native CALL_ASSEMBLER self-recursion fold already exempts that decline via SELFREC_CA_FOLD_ACTIVE; set the same flag around the admitted root-bridge inline's body sub-walk so the recursive residual runs concretely at the pre-execute site. Native only (cfg): the wasm always-portal path type-confuses the self-recursive inline, so it keeps the residual-fallback decline. bridge_recursion_overflow: bridges_compiled 0->1, loops_aborted 6->1, guard_failures 1307->360, byte-parity; fib_recursive unchanged. dynasm + cranelift 294/294. Assisted-by: Claude * jit: compile depth-2 carriers in the default p2 drain `drive_bridge_carrier_walk` (the default bridge-carrier consumer, p2 drain) only compiled `recipes.len() == 1` carriers; a `recipes.len() == 2` carrier fell through to `p2_drain_abort()` on every iteration. Add the depth-2 compile leg, gated on `carrier.recipes.len() == 2 && fbw_max_multiframe_depth() >= 2` (PYRE_FBW_MULTIFRAME_DEPTH, default 1): - pass `&carrier.recipes[..1]` (not `&[]`) as the deepest sub-walk's paused_parent_recipes so its in-callee guard snapshots encode the full [deepest, middle, root] chain; - after the deepest clean SubReturn, `drive_middle_frame_and_thread` sets up recipes[0], delivers the deepest result into its residual-call return register via `drive_bridge_middle_frame`, and returns the middle's own SubReturn; a non-portable middle shape returns None and falls through to the drain's journal-rollback abort epilogue; - replace the `== 1` compile gate with `== 1 || want_depth2`, feeding the threaded result through the existing `inject_root_call_result` + `full_body_walk_trace` root leg. The middle-drive mechanism is lifted from the non-default `drive_bridge_framestack_walk`; the drain variant's failure arms return None (census `P2Drain::Middle*`) instead of cut+reset so the epilogue rolls both frames' journaled stores back for the blackhole replay. Default-inert (depth 1): unchanged. At PYRE_FBW_MULTIFRAME_DEPTH=2 on depth2_inline_chain_typeflip / p2_local_result_bridge: loops_aborted 598 -> 0, guard_failures 120042 -> 802, bridges_compiled 1 -> 4, byte-parity held. check.py dynasm 294/294 (default) and dynasm+cranelift 294/294 (DEPTH=2). Assisted-by: Claude * jit: generalize the p2-drain compile leg to N recipes The default drain's depth-2 special case handled only `recipes.len() == 2`. Replace it with a loop over all paused middle frames so a depth-N carrier compiles: after the deepest callee's clean SubReturn, drive each middle from second-deepest (recipes[len-2]) out to the outermost (recipes[0]), delivering each callee's result into its caller's residual-call return register, then feed the outermost result through the existing `inject_root_call_result` + `full_body_walk_trace` root leg. `recipes[i]`'s paused parents are the shallower frames `recipes[..i]`; the deepest sub-walk's paused_parent_recipes generalize from `&recipes[..1]` to `&recipes[..len-1]`. - `drive_bridge_middle_frame` takes `paused_parent_recipes` (was hardcoded `&[]`) so a depth>2 middle's in-callee guard snapshots encode its own paused parents, not just the root; the framestack caller passes `&[]` (depth-2 only). - `drive_middle_frame_and_thread` takes `middle` + `paused_parents` instead of the whole carrier. - raise the depth clamp `.clamp(1, 2)` -> `.clamp(1, 3)` so PYRE_FBW_MULTIFRAME_DEPTH=3 is honored (default stays 1). Any non-portable middle shape returns None and falls through to the rollback abort epilogue, so a carrier we cannot compile deopts cleanly. New fixture depth3_inline_chain_typeflip.py (main -> f -> g -> h). At PYRE_FBW_MULTIFRAME_DEPTH=3 its `n_recipes=3` carriers were storming (loops_aborted 598, guard_failures 120042); now loops_aborted 0, guard_failures 802 (N-independent), bridges_compiled 1 -> 4, byte-parity 1800000.0. Default-inert. check.py dynasm+cranelift 295/295 at default, DEPTH=2, and DEPTH=3. Assisted-by: Claude * jit: raise multiframe depth clamp to 7 The N-recipe p2-drain compile leg handles any depth, but the clamp capped PYRE_FBW_MULTIFRAME_DEPTH at 3. Raise it to 7 (default stays 1). Verified depth 4-7 each form n_recipes=N carriers that compile instead of storming: loops_aborted 0, guard_failures 802 (depth-independent), bridges 4, byte-parity (depth4 2100000.0 / depth5 2400000.0 / depth6 2700000.0 / depth7 3000000.0). New fixture depth7_inline_chain_typeflip.py (main -> b -> c -> d -> e -> f -> g -> h). check.py dynasm+cranelift 295/295 at default and DEPTH=7. Assisted-by: Claude * jit: retire the framestack-walk bridge path The drain (`drive_bridge_carrier_walk`) now compiles N-deep carriers (recipes 1..=7), so the `PYRE_P2_DRAIN=0` framestack-walk escape hatch is redundant. It was correctness-buggy for a branchy inlined-callee continuation (the reason it was never the default) and its outer-continuation compile leg (`drive_outer_frame_continuation`) failed where the drain's `inject_root_call_result` + `full_body_walk_trace` root leg compiles. Delete `drive_bridge_framestack_walk`, `drive_outer_continuation_and_map`, `drive_outer_frame_continuation`, and the `p2_drain_enabled` gate; the carrier-resume dispatch now always drives the drain. PYRE_P2_DRAIN becomes a no-op. The branchy-callee bridge gap is unchanged (the drain deopts it cleanly); its regression bench comment and gate-triage.md are updated. Default behavior is unchanged (the drain was already the default). check.py dynasm+cranelift 296/296 at default and DEPTH=7; PYRE_P2_DRAIN=0 byte-parity; cargo test green. Assisted-by: Claude * jit: flip multiframe chain-inline depth default 1->7 fbw_max_multiframe_depth default 1->7 so a straight-line value-returning callee chain (b->c->...->h) inlines to its full depth, each extra level removing a residual call, instead of only the top inline level. Two callee shapes that lose at depth are capped separately so the chain depth rises without regressing them: - recursion-unroll depth split into fbw_max_rec_unroll_depth (default 1), a bound distinct from the chain-inline depth: a self-recursive callee (fib tree recursion) folds to CALL_ASSEMBLER past one level instead of unrolling each recursive call into a blackhole deopt. - callee_body_contains_raise caps a callee that raises inline to the top inline level (inline_depth 0): below an intermediate frame its unwind needs the cross-frame bridge (gh#343/gh#467) the drain cannot yet build, and inlining it deep drops the NestedBreakBridgeResume bridge. Both guardrails are no-ops at depth 1, so the prior default behavior is preserved when the depth is not raised. Synth output-parity clean across 284 benches at depths 2/3/7; check.py dynasm 297/297, cranelift 297/297. Assisted-by: Claude * style: rustfmt inline_call and residual_call blocks Line-break the bridge_rec_root_selfrec unsafe block and the ListAppendValue args.first().map chain to rustfmt 1.9.0 canonical form. No behavior change. Assisted-by: Claude
Summary
Lands the depth-1 self-recursive inline lift for exact-integer arithmetic
callees (
fib-shaped recursion), plus the guard-failure bridge resume pathbrought to
consume_boxestotality parity. Eight commits in two arcs.The depth-2 recursive inline this line of work originally targeted has since
landed on main (#674); what remains here is the depth-1 inline lift (measured
~6% on fib) and the resume-path orthodoxy the depth-2 shape motivated, now
carried to its endpoint: the bridge-entry seed is total (all three register
banks) so the coverage decline degrades to a
debug_assertwith a retainedcold airbag.
Arc 1 — depth-1 inline lift + resume-path orthodoxy (commits 1–5)
gate a plain-root-bridge self-recursive inline behind PYRE_FBW_BRIDGE_REC_INLINEdefault PYRE_FBW_BRIDGE_REC_INLINE onDecline uncovered bridge-entry live registersgeneralize the inline-recipe portal-callee shape toward consume_boxes totalitypreserve null bridge refs and per-guard declines1–2. Plain-root-bridge self-recursive inline lift
On a guard-failure bridge walk, an exact-integer arithmetic callee
(
args_all_builtin_integer+ aBinaryOpresidual body) was unconditionallykept as one residual call, because re-inlining it would create a second virtual
frame whose operand stack is not a red bridge input (an overflow path could
compile NULL vable stack slots into the bridge).
This admits that inline only on a plain root bridge —
!carrier_resume && !inline_subwalk && snapshot_sym non-null && framestack empty— uniform with aprimary trace: its second virtual frame is seeded and snapshot-covered exactly
as the loop's is, and the call falls through to the existing self-recursive
unroll gate and multiframe seed. The NULL-vable hazard is contained by the
seed-success precondition plus the
n_parents == n_calleessnapshot valvefallbacks. Behind
PYRE_FBW_BRIDGE_REC_INLINE(default-on;=0opts out).Measured: fib36 ~6.4% faster with the lift on vs
PYRE_FBW_BRIDGE_REC_INLINE=0.3 + 5. Bridge-entry live-register soundness net (consume_boxes parity)
run_perfn_walkdecodes the codewriterlive/set at a bridge entry pc anddeclines the walk — a non-abort
TraceAction::Declinethat falls back toresidual without charging
loops_aborted— when a live register the resumemarker names cannot be seeded from the bridge input banks. This mirrors the
consume_boxestotality contract (resume.py:1042-1057): every live registermust be fillable. Refined to upstream parity:
OpRef::NONE); a seededConstPtr(NULL)is a restored null value, not "uncovered" (_callback_r,resume.py:1032-1034);declined_bridge_guards), not a whole-greenkeypermanent disable — blackhole-once-retryable (
compile.py:701-717);Declineis a pre-trace coverage fallback, documented stats-neutral againstpyjitpl.py:2785-2789.4. Inline-recipe portal-callee generalization
reconstruct_inline_recipeclassified a paused portal callee with twoenumerated shapes: the canonical
[frame, ec]reds-only form and the reduced[frame]in-a-call form. A middle frame paused mid-expression keeps operandslive on the stack beside the reds, a wider live-ref set both shapes reject,
collapsing the carrier to a zero-recipe plain bridge.
Replace the two shapes with one predicate: accept any paused portal callee
whose every live non-red color maps to a semantic
locals_cells_stack_wslotthrough the per-pc pcdep inversion. The portal arm reconstructs each slot
semantic-ordered from the frame virtual's array; the mandatory-operand NONE
check still declines an uncovered slot. This subsumes both prior shapes and
narrows the gap to
consume_boxes, which fills every live register with noclassifier.
Arc 2 — S1 endpoint: total bridge seed (commits 6–8)
Thread Float bridge input bankDecline foreign bridge JitCode bodiesAssert bridge register seed totalityBrings the bridge-entry seed to full
consume_boxestotality(
resume.py:1017-1057—enumerate_varsfires_callback_i/_r/_fover everylive register of all three banks), so the arc-1 coverage decline degrades to an
assertion:
dispatch_via_miframealready accepts a Float bridge-input bank(
bridge_subwalk.rs:56, seededtop_regs_f) andsetup_bridge_symalreadydecodes Float colors into
registers_f; the sole caller passed&[]. Threadthe Float bank through
dispatch_perfn_frameand seed it exactly likeargboxes_i(_callback_f,resume.py:1036-1038). The categorical"any live Float is uncovered" leg becomes the same per-color probe as Int/Ref.
resume.py:1049-1056constructs the resumed frame against its ownJitCode body and applies the PC from that body's resume section. A guard from
a different installed body for the same code object carries a foreign
coordinate space; persist
frame0.jitcode_indexon the sym and decline(first-class runtime
Decline, retryable — never a green-key disable) when itdoes not match
pjc.jitcode.index(). CensusFbw::BridgeEntryForeignJitcode.defect:
debug_assert!("consume_boxes totality violated")enforces theendpoint in CI. The cold decline is retained as the release airbag
(
read_int_reg/read_float_regdo not NONE-check, so an uncovered colorwould record
OpRef::NONEinto an op → SIGSEGV/miscompile), with thecoverage census split per bank (
…Unseeded::{Ref,Int,Float}) for zero-fireevidence.
Verification
check.py --backend dynasm: 285/285 ALL PASSED.PYRE_JIT=0(6765 / 46368 / 317811).float_loop,spectral_norm,nbody): JIT vsPYRE_JIT=0byte-identical,
loops_aborted=0,internal_compile_panics=0, everyFbw::BridgeEntry*census leg zero-fire.fib_recursive,inline_helper, thethree float workloads,
PYRE_FBW_BRIDGE_REC_INLINE=1 PYRE_FBW_MULTIFRAME_DEPTH=2stress): the totalitydebug_assertdoes nottrip; all outputs correct.
🤖 Generated with Claude Code