Repository navigation
jit: inline nested int-arith calls in a carrier-resume bridge continuation (gh#343) - #748
Conversation
WalkthroughThe JIT now admits additional carrier-resume bridge sub-walks when snapshot state exists. A synthetic benchmark exercises a branch inside an inlined callee, while explanatory comments document the parity-suite guard. The former regression script is removed. ChangesBranchy callee bridge handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant drive_branch
participant compute_branch
participant inline_call
drive_branch->>compute_branch: evaluate each loop value
compute_branch->>inline_call: process branchy callee inline path
inline_call-->>compute_branch: admit carrier-resume bridge when snapshot exists
compute_branch-->>drive_branch: return branch result
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 |
![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: 2ba32fae90
ℹ️ 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 bridge_rec_lift_exempt = pre_execute | ||
| && fbw_bridge_rec_resid_enabled() | ||
| && BRIDGE_REC_LIFT_TOP_INLINE.with(|c| { | ||
| let lifted_code = c.get(); | ||
| let session = ctx.session.borrow(); | ||
| lifted_code != 0 | ||
| && session.framestack.len() == 1 | ||
| && session.framestack[0].w_code == lifted_code | ||
| }) | ||
| && hazard == Some(FbwInlineCalleeHazard::SelfRecursiveStatic); |
There was a problem hiding this comment.
This exemption is keyed only on the lifted frame's w_code plus the fact that the frame is statically self-recursive, but the pre-execute hook is invoked for every non-provably-side-effect-free residual in that frame. In a #704-lifted self-recursive integer callee that also does another residual effect (for example store_global, store_attr, logging, or a property/dunder call) before the recursive call, that unrelated residual now bypasses the nested-residual abort as well; if the bridge walk later declines, cleanup only rolls back the FBW journals, so the blackhole replay can re-run an effect that this guard was supposed to prevent. Please distinguish the actual self-recursive residual/CALL_ASSEMBLER case instead of exempting all residuals in the lifted frame.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit fe4238d). 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
|
2ba32fa to
0a7c1cf
Compare
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: 0a7c1cf94e
ℹ️ 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 try_multiframe = multiframe_eligible | ||
| && inline_depth < fbw_max_multiframe_depth() | ||
| && inline_depth < effective_multiframe_depth |
There was a problem hiding this comment.
When PYRE_FBW_MULTIFRAME_DEPTH is raised above 1 and a nested callee is strict_inlinable but contains a raise (for example a straight-line exception constructor/raise with no forward branch), this new cap is never consulted: try_multiframe is false for strict callees, the call continues through the strict path, and strict_seed below still uses the raw fbw_max_multiframe_depth(). That lets the cross-frame raise-unwind shape the new guard is trying to avoid still be inlined under an intermediate frame; please apply effective_multiframe_depth to the strict path as well, or reject/avoid seeding strict callees that contain raise.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
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: 7a7046a867
ℹ️ 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 Some(d) = crate::jitcode_runtime::decode_op_at(body_code, pc) else { | ||
| return false; | ||
| }; | ||
| if d.opname == "raise" { |
There was a problem hiding this comment.
With the new default depth allowing nested multiframe inlines, this scan only caps bodies containing raise/r; a callee that raises by bare raise in an exception handler uses the existing reraise/ opcode, so callee_body_contains_raise returns false and the callee can still be inlined below an intermediate frame. That re-enters the same unsupported cross-frame exception-unwind path the cap is meant to avoid for nested raising callees; please treat reraise as a raising opcode here as well.
Useful? React with 👍 / 👎.
7a7046a to
7f31265
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f3126535a
ℹ️ 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".
| chk.run_selfcheck( | ||
| "bridge_branchy_callee", | ||
| f"{B}/bridge_branchy_callee_regression.py", | ||
| 15, | ||
| skip_backends=("wasm",), |
There was a problem hiding this comment.
When check.py --no-fbw-inline-multiframe is used, pyre_env() forces PYRE_FBW_INLINE_MULTIFRAME=0 specifically to validate the flag-off fallback, but this newly added guard assumes compute_branch() is inlined via the multiframe path. With that path disabled, forward-branch callees are not multiframe_eligible (try_multiframe is false), so the script is no longer testing the gh#343 bridge continuation; it compares a residual-call branch path against the straight inlined path and can fail the rollback check. Please skip this selfcheck, or adjust its expectation, when FBW_INLINE_MULTIFRAME_OFF is set.
Useful? React with 👍 / 👎.
youknowone
changed the title
f21088f to
a96cfb7
Compare
…ation A branch (`PopJumpIfFalse` -> guard) inside an inlined callee makes the callee's own inner calls (`add`, `mul`) on the traced arm degrade to a residual `CALL_ASSEMBLER` (~100ns/crossing), while the primary loop inlines the same chain to a few ns (gh#343). The int-binop bridge-inline admission (`fbw_bridge_rec_inline_enabled` + `safe_root_bridge`) only fired for a plain ROOT bridge, never for the carrier-resume sub-walk (`drive_bridge_frame_subwalk`, which sets `carrier_resume`/`inline_subwalk`) that a branchy continuation traces through. Admit the carrier-resume sub-walk too: `safe_root_bridge = root_bridge || subwalk_admit`, where `subwalk_admit` holds when `carrier_resume` and a non-null snapshot_sym. The sub-walk drives its reconstructed frame(s) forward through the same metainterp the initial trace uses, so inlining a nested int-arith call there is uniform with a primary trace — residualizing was the deviation. Assisted-by: Claude
…suite Add pyre/bench/synth/bridge_branchy_callee.py: a branch inside an inlined callee (gh#343) whose rare arm guard-fails, exercising the carrier-resume sub-walk's int-arith inline of the branchy continuation. It prints a deterministic sum and is gated against pypy by `# pyre-check: max-pypy-ratio=8`. The synthetic suite times pyre-JIT vs pypy (native backends, startup-subtracted): the fix holds at 1.4-1.7x (dynasm) / 3.3-3.6x (cranelift), while a decline that keeps every crossing interpreted balloons it to ~30x, so the ratio catches the regression with margin. wasm is exempt (guest has no time.perf_counter()). Drops the old bridge_branchy_callee_regression.py run_selfcheck. check.py dynasm + cranelift. Assisted-by: Claude
a96cfb7 to
fe4238d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1458-1474: The root-bridge self-recursion exemption is incorrectly
enabled for carrier-resume sub-walks through safe_root_bridge. Update the flag
computation near root_bridge and subwalk_admit so bridge_rec_root_selfrec
remains true only for the plain root_bridge, while carrier-resume entries can
still use subwalk_admit for inline admission; preserve the existing
self-recursive hazard checks for carrier-resume paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47f3c814-805b-432a-acfc-ace3ed13dd2d
📒 Files selected for processing (4)
pyre/bench/bridge_branchy_callee_regression.pypyre/bench/synth/bridge_branchy_callee.pypyre/check.pypyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
💤 Files with no reviewable changes (1)
- pyre/bench/bridge_branchy_callee_regression.py
| // A depth-1 carrier-resume sub-walk (`drive_bridge_frame_subwalk`) drives | ||
| // its reconstructed frame as the sub-walk root with an empty framestack, | ||
| // so it is uniform with a plain root bridge for the multiframe seed — | ||
| // admit it too so a rare guard-bridge continuation inlines its nested | ||
| // int-arith calls instead of residualizing them (the gh#343 branchy-callee | ||
| // cost). | ||
| let root_bridge = !ctx.fbw_mode.carrier_resume | ||
| && !ctx.fbw_mode.inline_subwalk | ||
| && !ctx.fbw_mode.snapshot_sym.is_null() | ||
| && ctx.session.borrow().framestack.is_empty(); | ||
| // A carrier-resume sub-walk (`drive_bridge_frame_subwalk`) drives its | ||
| // reconstructed frame(s) forward through the same metainterp the initial | ||
| // trace uses, so its inline of a nested int-arith call is the SAME as a | ||
| // primary trace's — admit it (any inline depth) so the rare guard-bridge | ||
| // continuation inlines instead of residualizing. | ||
| let subwalk_admit = ctx.fbw_mode.carrier_resume && !ctx.fbw_mode.snapshot_sym.is_null(); | ||
| let safe_root_bridge = root_bridge || subwalk_admit; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant implementation and usages without running repo code.
fd -a 'inline_call.rs' . | sed 's#^\./##'
echo '--- find file line count ---'
wc -l pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
echo '--- outline around relevant file ---'
ast-grep outline pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs --view summary || true
echo '--- inspect lines 1430-1500 ---'
sed -n '1430,1500p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | cat -n | sed 's/^/L /'
echo '--- inspect lines 2280-2340 ---'
sed -n '2280,2340p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | cat -n | sed 's/^/L /'
echo '--- search symbol usages ---'
rg -n "bridge_rec_root_selfrec|SelfRecCaFoldGuard|safe_root_bridge|subwalk_admit|root_bridge|carrier_resume|drive_bridge_frame_subwalk" pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rsRepository: youknowone/pyre
Length of output: 10106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inspect carrier-resume mode context ---'
sed -n '1360,1515p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | cat -n | sed 's/^/L /'
echo '--- inspect self-rec guard definition and nested-residual decline ---'
rg -n "struct SelfRecCaFoldGuard|impl SelfRecCaFoldGuard|SelfRecCaFoldGuard|fbw_abort_nested_unjournaled_residual|nested_unjournaled_residual|SELFREC_CA_FOLD_ACTIVE|bridge_rec_root_selfrec" pyre/pyre-jit-trace/src -S
echo '--- inspect related files from rg context ---'
for f in $(rg -l "SelfRecCaFoldGuard|fbw_abort_nested_unjournaled_residual|SELFREC_CA_FOLD_ACTIVE" pyre/pyre-jit-trace/src); do
echo "### $f"
wc -l "$f"
rg -n -C 8 "SelfRecCaFoldGuard|fbw_abort_nested_unjournaled_residual|SELFREC_CA_FOLD_ACTIVE" "$f"
doneRepository: youknowone/pyre
Length of output: 27206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inspect carrier-resume mode context before selfrec block ---'
sed -n '1360,1465p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | cat -n | sed 's/^/L /'
echo '--- inspect walk body around subcall inline continuation ---'
sed -n '1515,1780p' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | cat -n | sed 's/^/L /'
echo '--- inspect self-rec guard and nested-residual decline ---'
rg -n "struct SelfRecCaFoldGuard|impl SelfRecCaFoldGuard|SelfRecA|fbw_abort_nested_unjournaled_residual|fbw_abort_self_recursive|nested_unjournaled_residual|UNRECODABLE_MARKER|SAFE_FOLD" pyre/pyre-jit-trace/src -S
echo '--- list candidate helper files ---'
fd -a '.*\.rs$' pyre/pyre-jit-trace/src | xargs rg -l "SelfRecCaFoldGuard|fbw_abort_nested_unjournaled_residual|UNRECODABLE_MARKER|SAFE_FOLD"Repository: youknowone/pyre
Length of output: 24984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inspect fbw_abort_nested_unjournaled_residual implementation ---'
sed -n '1266,1383p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs | cat -n | sed 's/^/L /'
echo '--- inspect SelfRecCaFoldGuard RAII implementation ---'
sed -n '4638,4674p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs | cat -n | sed 's/^/L /'
echo '--- inspect residual call sites just before nested declarations ---'
for f in pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs; do
echo "### $f"
rg -n -B 10 -A 12 "fbw_abort_nested_unjournaled_residual\\(" "$f"
done
echo '--- read-only behavioral probe: derive control-flow condition reachability ---'
python3 - <<'PY'
cases = [
{"carrier_resume": False, "inline_subwalk": False, "snapshot_null": False, "framestack_empty": True},
{"carrier_resume": True, "inline_subwalk": True, "snapshot_null": False, "framestack_empty": True},
{"carrier_resume": True, "inline_subwalk": False, "snapshot_null": False, "framestack_empty": True},
]
for case in cases:
root_bridge = (
not case["carrier_resume"]
and not case["inline_subwalk"]
and not case["snapshot_null"]
and case["framestack_empty"]
)
subwalk_admit = case["carrier_resume"] and not case["snapshot_null"]
safe_root_bridge = root_bridge or subwalk_admit
print(case, "root_bridge", root_bridge, "subwalk_admit", subwalk_admit, "safe_root_bridge", safe_root_bridge)
PYRepository: youknowone/pyre
Length of output: 14279
Do not apply the root-bridge self-recursion exemption to carrier-resume sub-walks.
subwalk_admit already allows carrier-resume entries to inline, but it also makes safe_root_bridge true for non-root carrier bridges, so bridge_rec_root_selfrec enables SelfRecCaFoldGuard at Line 2318. That guard bypasses the self-recursive hazard check in fbw_abort_nested_unjournaled_residual, and the documented safety argument only applies to the plain root bridge. Narrow the flag to root_bridge, or add a carrier-resume-specific proof and regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 1458 -
1474, The root-bridge self-recursion exemption is incorrectly enabled for
carrier-resume sub-walks through safe_root_bridge. Update the flag computation
near root_bridge and subwalk_admit so bridge_rec_root_selfrec remains true only
for the plain root_bridge, while carrier-resume entries can still use
subwalk_admit for inline admission; preserve the existing self-recursive hazard
checks for carrier-resume paths.
…ume keep Record in the PYRE_P2_DRAIN retirement note that the branchy inlined-callee continuation gap's plain-branch subcase is closed unconditionally (the carrier-resume sub-walk int-binop inline landed without a gate in #748), and its exception-unwind subcase is handled by the now-unconditional try-block callee inline + carrier-boundary raise delivery (PYRE_FBW_TRYBLOCK_INLINE / PYRE_FBW_CARRIER_RAISE retired). Add PYRE_CARRIER_EXC_RESUME to Judged KEEP (default-off; the bridge_guard_exc GC-rooting and the unconditional execute_ll_raised exception assign are the parity gaps to close before it is enabled by default). Assisted-by: Claude
…e handler fix (#759) * jit: retire two default-on FBW inline gates PYRE_FBW_TRYBLOCK_INLINE and PYRE_FBW_CARRIER_RAISE both defaulted on with no remaining reason to toggle off once verified, so make their behavior unconditional and drop the gates: - a try-block CALL whose except handler rejoins a loop always multi-frame-inlines; decline_inline_caller_frame_for_catch_marker now runs the any-loop rejoin test directly instead of branching between it and a specific-loop fallback. - the depth-2 carrier-boundary raise delivery in drive_bridge_carrier_walk is always active. Deletes the now-dead exc_handler_rejoins_specific_loop and enclosing_loop_header_jit_pc (only the carrier-raise-off branch used them) and the now-unused call_jit_pc parameter of decline_inline_caller_frame_for_catch_marker. PYRE_CARRIER_EXC_RESUME stays (default-off lever). exceptions / exception_args_virtual / break_except_live_local bit-exact with loops_aborted=0; bridge_branchy_callee regression passes; check.py dynasm 303/303, cranelift 303/303, wasm 300/300. Assisted-by: Claude * docs: gate-triage note — branchy subcase closures and carrier-exc-resume keep Record in the PYRE_P2_DRAIN retirement note that the branchy inlined-callee continuation gap's plain-branch subcase is closed unconditionally (the carrier-resume sub-walk int-binop inline landed without a gate in #748), and its exception-unwind subcase is handled by the now-unconditional try-block callee inline + carrier-boundary raise delivery (PYRE_FBW_TRYBLOCK_INLINE / PYRE_FBW_CARRIER_RAISE retired). Add PYRE_CARRIER_EXC_RESUME to Judged KEEP (default-off; the bridge_guard_exc GC-rooting and the unconditional execute_ll_raised exception assign are the parity gaps to close before it is enabled by default). Assisted-by: Claude * jit: set class_of_last_exc_is_const on the carrier-raise handler entry The carrier-boundary raise handler-entry branch in dispatch_via_miframe seeded last_exc_value + the handler-entry operand stack but left class_of_last_exc_is_const at its stale pre-bridge value, unlike the sibling exc_edge_catch_target branch and the walk-level SubRaise routing (opimpl_raise), which set it true. The seed's exception is a const-class raise (the callee's inline RAISE recorded a NewWithVtable of a known class), so a reraise in the handler read the standing exception state from the stale flag. Set it true to match the siblings. Document the default-off PYRE_CARRIER_EXC_RESUME pre-flip-on parity gaps (gate-triage.md, trace_ctx.rs): the bridge_guard_exc GCREF-as-i64 is not a traced root and the seed's execute_ll_raised assign is guarded by is_null rather than unconditional; both are inert while the gate is off. reraise-in-handler repro bit-exact (875000) on both backends; check.py dynasm 301/301, cranelift 301/301, wasm 297/297. Assisted-by: Claude
Extends the carrier-resume sub-walk to inline a rare guard-bridge continuation's
nested exact-integer arithmetic calls instead of residualizing them as a
CALL_ASSEMBLER, closing the gh#343 plain-branch inlined-callee cost. Rebasedonto current
main— the depth-N bridge-carrier convergence this branchpreviously carried landed separately (#749), so this PR now contains only the
gh#343 fix.
Root cause: a branch (
PopJumpIfFalse-> guard) inside an inlined callee makesthe callee's OWN inner calls (
add,mul) on the traced arm degrade to aresidual
CALL_ASSEMBLER(~100ns/crossing) while the primary loop inlines thesame chain to a few ns. The int-binop bridge-inline admission only fired for a
plain ROOT bridge (
safe_root_bridgerequired!carrier_resume && !inline_subwalk), never for the carrier-resume sub-walk(
drive_bridge_frame_subwalk, which sets both) that a branchy continuationtraces through. The sub-walk drives its reconstructed frame through the same
metainterp the initial trace uses, so inlining the int-arith chain there is
uniform with a primary trace — residualizing was the deviation.
Commits:
split
safe_root_bridgeintoroot_bridge || subwalk_admit; admit thecarrier-resume sub-walk unconditionally (
carrier_resume+ non-nullsnapshot_sym). No env gate — PyPy has no such switch, so the fix is always on.
add
pyre/bench/synth/bridge_branchy_callee.py, a deterministic-sum fixturegated against pypy by
# pyre-check: max-pypy-ratio=8on the native backends(wasm exempt, its guest has no
time.perf_counter()).Verification:
synth/bridge_branchy_calleeholds at 1.4-1.7x (dynasm) / 3.3-3.6x(cranelift) vs pypy; a decline that keeps every crossing interpreted
balloons to ~30x and FAILs the 8x gate
branchy driver goes 176ns -> 80ns/iter; a leaf branchy callee (no inner
calls) is unchanged, and a straight (non-branch) callee does not regress
_handle_guard_failure->prepare_resume_from_failure->interpret()resume(Codex parity review clean on the prior revision; removing the env gate makes
it strictly more orthodox — PyPy uses no such runtime switch)
The exception-unwind continuation subcase of gh#343 (a raising callee, not a
data branch) is unchanged and stays with the epic.
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Tests
Documentation