jit: guard the residual-call dispatch table's covered width and drop its per-call allocations - #1006
Conversation
…width `dispatch_classes_body!` enumerates one `extern "C"` signature per ordered argument-class sequence: every sequence up to 5 arguments, plus the all-Int sequences on to MAX_HOST_CALL_ARITY. That float-carrying width was stated nowhere on the descr-build side, so a wider float-bearing signature would reach the table's catch-all panic only at the first deopt that dispatches it. Add MAX_FLOAT_CARRYING_CALL_ARITY = 5 next to BhCallDescr and a debug_assert on its three constructors. Signatures past MAX_HOST_CALL_ARITY are exempt: residual_call.rs declines to emit the call at that width, so no blackhole dispatches them. Add call_stub tests for a float in the last covered slot and for the refusal one argument past it. Assisted-by: Claude
…typed-call type list covers every slot `collect_call_args` returned two `Vec`s, allocated on every residual call the blackhole dispatches. Return a fixed-size `CallArgs` buffer sized MAX_HOST_CALL_ARITY — the bound the dispatch table's widest arm already stops at — and update the dynasm, cranelift and shared `*_by_classes` call sites. The wasm host-trampoline path keeps `collect_call_args_positional`. `arg_classes_from_types` read `arg_types` with `.get(i)` and mapped a missing slot to `ArgClass::Int`, which would put an undescribed Float argument in an integer register. Both callers take the positional arguments and the type list from the same calldescr, so debug_assert that the type list is no shorter. 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 (5)
Walkthrough
ChangesResidual-call dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5879f6fd89
ℹ️ 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".
| return; | ||
| } | ||
| debug_assert!( | ||
| !arg_classes.contains('f') || arity <= MAX_FLOAT_CARRYING_CALL_ARITY, |
There was a problem hiding this comment.
Support wide float descriptors instead of rejecting them
In debug builds, any valid residual-call descriptor with 6–16 arguments and at least one f64 now panics during construction, even though it is within MAX_HOST_CALL_ARITY and compiled traces/wasm can place the signature. This replaces upstream's per-descriptor stub support with an artificial rejection; release builds merely defer the same unsupported signature to a deoptimization-time panic. Extend the native dispatcher or use the documented ABI-adapter approach rather than rejecting these descriptors.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 5879f6f). 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
|
Follow-up to #1002 (Codex P1 + two CodeRabbit findings on that PR).
#1002 replaced the bucket-keyed residual-call dispatch with a class-sequence
table, matching
descr.py:574/descr.py:604-605 create_call_stubdeclaration order. Three loose ends were left on it.
1. The table's covered width was stated nowhere on the descr-build side
dispatch_classes_body!enumerates oneextern "C"signature per orderedargument-class sequence: every sequence up to 5 arguments, plus the
all-
Intsequences on toMAX_HOST_CALL_ARITY. Nothing said so wherecalldescrs are minted, so adding an
f64parameter to an already-wide helperwould surface only as the table's catch-all panic — at the first deopt that
dispatches that call, far from the edit that caused it.
MAX_FLOAT_CARRYING_CALL_ARITY = 5now sits next toBhCallDescr, with adebug_asserton all three of its constructors. Signatures pastMAX_HOST_CALL_ARITYare deliberately exempt:residual_call.rsalreadydeclines to emit the call at that width, so no blackhole ever dispatches them
and asserting would turn an orderly decline into a debug panic.
Upstream needs no such bound —
create_call_stubsource-generatesFuncType(ARGS, RESULT)per calldescr at translation time, so every sequencehas a stub. Lifting it here means an ABI adapter for signatures known only at
run time; the catch-all's TODO records that convergence path.
2. Two heap allocations per residual call
collect_call_argsreturned twoVecs on a path the blackhole takes for everyresidual call. It now fills a fixed
CallArgsbuffer sizedMAX_HOST_CALL_ARITY— the same bound the widest arm stops at, so a signaturethat does not fit has no arm either. The wasm host-trampoline path keeps
collect_call_args_positionalunchanged.3. An undescribed argument slot silently became an integer
arg_classes_from_typesreadarg_typeswith.get(i)and mappedNonetoArgClass::Int— exactly the mis-placement #1002 exists to prevent, had theundescribed slot been a
Float. Both callers take the positional arguments andthe type list from the same calldescr, so that is now a
debug_assert.Tests
(i64,i64,i64,i64,f64));collect_call_argsrefuses more arguments than the table covers.Verification
cargo test -p majit-backend -p majit-metainterp -p majit-translate -p pyre-jit-trace— 0 failed. The debug build exercisesdebug_assert_dispatchableagainst the whole in-tree descr universe.python3 pyre/check.py --backend dynasm,cranelift,wasm— ALL PASSED: dynasm 370/370, cranelift 370/370, wasm 366/366.— authored by Claude
Summary by CodeRabbit
Bug Fixes
Performance