jit: dispatch residual calls in arg_classes declaration order - #1002
Conversation
…note `residual_call_float_canonical_via_target_with_effect_info` bakes `target.concrete_ptr` as the funcbox that `executor.rs`'s `Type::Float` arm reads back through an `extern "C" fn(..) -> f64` ABI. That is correct only while `concrete_ptr` is the raw callee; the macro's `_concrete` wrapper has an `-> i64` signature carrying `f64::to_bits`. Only the `*_float_wrapped` call policies mint that divergence and no crate declares one, so state the invariant as a `debug_assert_eq!` at the bake site instead of leaving it resting on that absence. `call_float_function`'s note claimed no interleaved float-returning helper exists. `jit_math_ldexp_raw(f64, i64) -> f64` is one, minted with `arg_types = [Float, Int]` in the walker specializer. Record the actual reason it does not reach this seam: it is recorded onto the trace via `TraceCtx::call_float_typed_with_effect` and consumed by the backends and `bh_call_f_by_classes`, while this seam is fed from the jitcode descr pool. Add tests covering the interleaving refusal and the accepted `[Int, Float]` order. Assisted-by: Claude
`dispatch_arity_body!` recovered the callee signature from the bucket counts
`(n_int, n_float)`, so a callee declared `fn(f64, i64)` was dispatched as
`fn(i64, f64)`. `call_float_function` refused such a signature outright.
`descr.py:574 create_call_stub` builds the call as
`", ".join([process(c) for c in self.arg_classes])`, and `process(c)` walks
`arg_classes` in declaration order pulling the next item out of the matching
bank; `descr.py:604-605` builds `FuncType(ARGS, RESULT)` over the same ordered
list. Upstream buckets for transport and un-buckets before the machine call.
`test_descr.py::test_call_stubs_2` runs `arg_classes == "fr"` through
`call_stub_f`. There is no arity ceiling and no ordering restriction upstream.
Replace the bucket-keyed table with a class-sequence-keyed one: `ArgClass`
carries the two C-ABI register classes, and `dispatch_classes_body!` matches on
the ordered class slice — every sequence of length 0..=5 plus the integer-only
tail through 16, 74 arms. `collect_call_args` now returns the ordered class
list and a positional argument list. `bh_call_{i,f,v}_dispatch` take
`(classes, args)`; the dynasm and cranelift `bh_call_*` overrides and
`call_float_function` pass them through. The float-after-int refusal in
`call_float_function` is deleted.
Add a port of `test_call_stubs_2` plus two further interleaved shapes.
Assisted-by: Claude
Six assertions in comments this branch added did not survive checking against the code they name: * `add_fn_ptr` was cited at `jitcode/assembler.rs:4617`; this branch inserted 16 lines earlier in that file, moving it to 4633. * The `f64::to_bits`-packing `_concrete` wrapper was attributed to `#[jit_module]` twice. It is emitted by `emit_helper_call_target_fn` (`majit-macros/src/lib.rs:605-614`), reached from the per-helper policy attributes. Record also that `jit_release_gil` reaches it through `_call_aroundstate_target_<name>`, whose first element is that wrapper. * "that i64 wrapper" in the CALL_ASSEMBLER sentence corefered to `_concrete`; that arm wants its own `call_assembler` entry wrapper, a different one. * The NULL-Ref fold refusal claimed upstream's optimizer inserts `guard_nonnull` ahead of a pointer-deref residual call. `GUARD_NONNULL` is emitted from `pyjitpl.py:558-575 _establish_nullity`, i.e. from the traced program's own null test; upstream simply never derives that NULL. * The same comment named `PyFrame.f_back`; the registered field is `PyFrame.f_backref`. It carries `immutable = false`, so `is_always_pure()` is false and the constant-pool arm is unreachable for it — the NULL is stamped onto the recorded OpRef via `set_opref_concrete`. * Two test doc comments stated that reading the integer return register returned the argument still sitting in it. That register is undefined after a call to an `f64`-returning callee. Also narrow the `executor.py:66-68` citation to the result half and cite `descr.py:604-605` for the argument half. Assisted-by: Claude
WalkthroughThe PR replaces bucketed call dispatch with declaration-order ChangesTyped ABI dispatch
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
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: 46ef114881
ℹ️ 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".
| classes => { | ||
| // TODO: upstream | ||
| // `rpython/jit/backend/llsupport/descr.py:590-602 | ||
| // create_call_stub` generates a per-calldescr stub at | ||
| // translation time so any (ni, nf) combination has a | ||
| // matching extern "C" signature. Rust has no | ||
| // translation-time codegen equivalent, so the dispatch | ||
| // is a hand-rolled arity table. Convergence path: | ||
| // wire libffi (or an ABI adapter) so any arity is | ||
| // dispatchable; until then, callees outside the | ||
| // table panic instead of silently corrupting registers. | ||
| // `rpython/jit/backend/llsupport/descr.py:574` / | ||
| // `descr.py:604-605 create_call_stub` generates a | ||
| // per-calldescr stub at translation time, so every class |
There was a problem hiding this comment.
For any residual callee containing a float among six or more parameters, this new table reaches the catch-all and panics, even though the upstream create_call_stub generates a callable for every arg_classes sequence. Replacing the bucketed dispatcher with another finite hand-written table therefore still makes valid residual calls fail; generate the ordered per-descriptor stub, or use an equally general ABI adapter, rather than limiting mixed signatures to five arguments.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
`executor.py:52-78` hands `descr` to `cpu.bh_call_i`, `cpu.bh_call_r` and `cpu.bh_call_v` exactly as it does to `cpu.bh_call_f`, so the generated stub's parameter types come from `arg_classes` on every path — a Float parameter is class `'f'` whatever the result type is. pyre routed only the Float-result arm that way. The Int/Ref arm called `call_int_function`, which takes no `arg_types` and passes every argument as `i64`. The Void arm matched one hardcoded shape, `[Ref, Int, Int, Float]`, and fell back to the same all-`i64` path for every other float-carrying signature. Both handed a Float parameter to the callee in an integer register. Extract `arg_classes_from_types` from `call_float_function`, add `call_int_function_typed`, and rewrite `call_void_function_typed` on top of it; point `execute_pure_call` and `execute_residual_call` at both. `call_int_function` stays for the seams that hold no descr — `execute_varargs`'s portal runner and the CALL_ASSEMBLER family, whose entry wrapper is `extern "C" fn(..) -> i64` by construction. Both new tests were confirmed to fail against the previous arms. A callee taking a single `f64` cannot make that check: the value it wants may be left in xmm0 by the caller, so the first draft passed against the broken path. Both tests therefore take `(f64, i64)` and encode both arguments in the result. Assisted-by: Claude
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
majit/majit-backend/src/call_stub.rs (1)
35-1054: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep float arg handling for high-arity
bh_callpaths.
dispatch_classes_bodynow coversFloatarguments only through arity 5. For 6+ arguments it only has all-Intmatch arms, so anyArgClass::Floatin the last positions hits the fallbackpanic!. Add the sameextern "C" fndispatch branches back for 6+ arityFloatarguments, or handle them in the fallback without panicking.🤖 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 `@majit/majit-backend/src/call_stub.rs` around lines 35 - 1054, Extend dispatch_classes_body to support ArgClass::Float combinations for arities 6 through 16, matching the existing lower-arity extern "C" dispatch behavior and converting float bit patterns via f64::from_bits. Ensure high-arity calls with any float positions no longer reach the panic fallback, while preserving the existing all-Int branches.majit/majit-metainterp/src/executor.rs (1)
789-837: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-extract the Charon ULLBC before rebuilding rtyper/annotator.
Interpreter/JIT Rust source changed in
majit/majit-metainterp/src/executor.rs,majit/majit-metainterp/src/jitcode/assembler.rs,majit/majit-backend-cranelift/src/compiler.rs, andmajit/majit-backend-dynasm/src/runner.rs. The trackedmajit/charon-corpus/corpus.ullbcis unchanged, so regenerate the ULLBC for these updated crates and rebuild the annotator/rtyper prepass instead of shipping stale Charon artifacts.🤖 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 `@majit/majit-metainterp/src/executor.rs` around lines 789 - 837, Re-extract the Charon ULLBC and rebuild the annotator/rtyper prepass for the updated Rust sources: majit/majit-metainterp/src/executor.rs (789-837), majit/majit-metainterp/src/jitcode/assembler.rs (3655-3673), majit/majit-backend-cranelift/src/compiler.rs (16809-16919), and majit/majit-backend-dynasm/src/runner.rs (3342-3451). Refresh the tracked corpus.ullbc artifact instead of shipping the stale version.Source: Coding guidelines
🤖 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 `@majit/majit-backend/src/call_stub.rs`:
- Around line 1174-1178: Update collect_call_args to replace the per-call Vec
allocations for classes and args with fixed-size buffers bounded by the existing
maximum arity, or the project’s small-vector equivalent. Preserve the current
population and indexing behavior for bh_call_i, bh_call_r, bh_call_f, and both
void residual-call paths, matching arg_classes_from_types’ allocation-free
behavior.
- Around line 1377-1432: Add a #[should_panic] test in the existing tests module
that invokes the relevant call-stub dispatcher with a six-argument ArgClass
sequence containing exactly one Float, such as the unsupported shape handled by
the fallback arm, and assert the documented panic message. Reuse the dispatcher
and argument representation used by the nearby call_stub_* tests, ensuring the
test verifies that this sequence panics instead of being dispatched.
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 8949-8975: Extend the regression tests near
interleaved_int_after_float_dispatches_in_declaration_order and
float_after_int_dispatches with one test for call_int_function_typed and one for
call_void_function_typed. Use interleaved signatures containing a Float
parameter and assert the integer result or void callee behavior, respectively,
matching the existing call_float_function coverage.
- Around line 8486-8505: Add a debug_assert_eq! at the start of
arg_classes_from_types to require arg_types.len() == args_len before class
generation. Keep the existing arity assertion and per-slot mapping unchanged.
---
Outside diff comments:
In `@majit/majit-backend/src/call_stub.rs`:
- Around line 35-1054: Extend dispatch_classes_body to support ArgClass::Float
combinations for arities 6 through 16, matching the existing lower-arity extern
"C" dispatch behavior and converting float bit patterns via f64::from_bits.
Ensure high-arity calls with any float positions no longer reach the panic
fallback, while preserving the existing all-Int branches.
In `@majit/majit-metainterp/src/executor.rs`:
- Around line 789-837: Re-extract the Charon ULLBC and rebuild the
annotator/rtyper prepass for the updated Rust sources:
majit/majit-metainterp/src/executor.rs (789-837),
majit/majit-metainterp/src/jitcode/assembler.rs (3655-3673),
majit/majit-backend-cranelift/src/compiler.rs (16809-16919), and
majit/majit-backend-dynasm/src/runner.rs (3342-3451). Refresh the tracked
corpus.ullbc artifact instead of shipping the stale version.
🪄 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: 52633b55-3000-4f16-aae1-d9fb105b85c9
📒 Files selected for processing (9)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend/src/call_stub.rsmajit/majit-metainterp/src/executor.rsmajit/majit-metainterp/src/jitcode/assembler.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
| let mut classes: Vec<ArgClass> = Vec::with_capacity(arg_classes.len()); | ||
| let mut args: Vec<i64> = Vec::with_capacity(arg_classes.len()); | ||
| let mut ii = 0usize; | ||
| let mut ri = 0usize; | ||
| let mut fi = 0usize; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid two heap allocations per residual call.
collect_call_args allocates two Vecs on every residual call. bh_call_i, bh_call_r, bh_call_f, and the void path in both backends call it on the residual-call hot path. The metainterpreter sibling arg_classes_from_types in majit/majit-metainterp/src/pyjitpl/dispatch.rs (lines 8482-8489) already returns a fixed-size buffer to stay allocation-free.
Use fixed-size buffers bounded by the existing maximum arity, or a small-vector type, so both seams have the same allocation behavior.
🤖 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 `@majit/majit-backend/src/call_stub.rs` around lines 1174 - 1178, Update
collect_call_args to replace the per-call Vec allocations for classes and args
with fixed-size buffers bounded by the existing maximum arity, or the project’s
small-vector equivalent. Preserve the current population and indexing behavior
for bh_call_i, bh_call_r, bh_call_f, and both void residual-call paths, matching
arg_classes_from_types’ allocation-free behavior.
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| extern "C" fn f2(a: f64, b: *const i64) -> f64 { | ||
| a + unsafe { *b } as f64 | ||
| } | ||
|
|
||
| extern "C" fn int_float_int(a: i64, b: f64, c: i64) -> i64 { | ||
| a + b as i64 * 10 + c * 100 | ||
| } | ||
|
|
||
| extern "C" fn float_float_int(a: f64, b: f64, c: i64) -> f64 { | ||
| a + b * 10.0 + c as f64 * 100.0 | ||
| } | ||
|
|
||
| /// Rust port of | ||
| /// `rpython/jit/backend/llsupport/test/test_descr.py::test_call_stubs_2`. | ||
| #[test] | ||
| fn call_stub_f_interleaved_float_ref_preserves_declaration_order() { | ||
| let b = [1_i64]; | ||
| let result = unsafe { | ||
| bh_call_f_dispatch( | ||
| f2 as *const () as usize, | ||
| &[ArgClass::Float, ArgClass::Int], | ||
| &[3.5_f64.to_bits() as i64, b.as_ptr() as i64], | ||
| ) | ||
| }; | ||
| assert_eq!(result, 4.5); | ||
| } | ||
|
|
||
| #[test] | ||
| fn call_stub_i_interleaved_int_float_int_preserves_declaration_order() { | ||
| let result = unsafe { | ||
| bh_call_i_dispatch( | ||
| int_float_int as *const () as usize, | ||
| &[ArgClass::Int, ArgClass::Float, ArgClass::Int], | ||
| &[1, 2.0_f64.to_bits() as i64, 3], | ||
| ) | ||
| }; | ||
| assert_eq!(result, 321); | ||
| } | ||
|
|
||
| #[test] | ||
| fn call_stub_f_interleaved_float_float_int_preserves_declaration_order() { | ||
| let result = unsafe { | ||
| bh_call_f_dispatch( | ||
| float_float_int as *const () as usize, | ||
| &[ArgClass::Float, ArgClass::Float, ArgClass::Int], | ||
| &[1.0_f64.to_bits() as i64, 2.0_f64.to_bits() as i64, 3], | ||
| ) | ||
| }; | ||
| assert_eq!(result, 321.0); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for an unsupported class sequence.
The tests cover three supported interleaved shapes. They do not cover the fallback arm at lines 1055-1070. A sequence such as six arguments with one Float must panic with the documented message rather than dispatch.
Add a #[should_panic] test so the panic contract stays locked.
💚 Proposed test
+ #[test]
+ #[should_panic(expected = "unsupported arg class sequence")]
+ fn call_stub_unsupported_class_sequence_panics() {
+ let classes = [
+ ArgClass::Int,
+ ArgClass::Int,
+ ArgClass::Int,
+ ArgClass::Int,
+ ArgClass::Int,
+ ArgClass::Float,
+ ];
+ let args = [0_i64; 6];
+ unsafe { bh_call_v_dispatch(int_float_int as *const () as usize, &classes, &args) };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| extern "C" fn f2(a: f64, b: *const i64) -> f64 { | |
| a + unsafe { *b } as f64 | |
| } | |
| extern "C" fn int_float_int(a: i64, b: f64, c: i64) -> i64 { | |
| a + b as i64 * 10 + c * 100 | |
| } | |
| extern "C" fn float_float_int(a: f64, b: f64, c: i64) -> f64 { | |
| a + b * 10.0 + c as f64 * 100.0 | |
| } | |
| /// Rust port of | |
| /// `rpython/jit/backend/llsupport/test/test_descr.py::test_call_stubs_2`. | |
| #[test] | |
| fn call_stub_f_interleaved_float_ref_preserves_declaration_order() { | |
| let b = [1_i64]; | |
| let result = unsafe { | |
| bh_call_f_dispatch( | |
| f2 as *const () as usize, | |
| &[ArgClass::Float, ArgClass::Int], | |
| &[3.5_f64.to_bits() as i64, b.as_ptr() as i64], | |
| ) | |
| }; | |
| assert_eq!(result, 4.5); | |
| } | |
| #[test] | |
| fn call_stub_i_interleaved_int_float_int_preserves_declaration_order() { | |
| let result = unsafe { | |
| bh_call_i_dispatch( | |
| int_float_int as *const () as usize, | |
| &[ArgClass::Int, ArgClass::Float, ArgClass::Int], | |
| &[1, 2.0_f64.to_bits() as i64, 3], | |
| ) | |
| }; | |
| assert_eq!(result, 321); | |
| } | |
| #[test] | |
| fn call_stub_f_interleaved_float_float_int_preserves_declaration_order() { | |
| let result = unsafe { | |
| bh_call_f_dispatch( | |
| float_float_int as *const () as usize, | |
| &[ArgClass::Float, ArgClass::Float, ArgClass::Int], | |
| &[1.0_f64.to_bits() as i64, 2.0_f64.to_bits() as i64, 3], | |
| ) | |
| }; | |
| assert_eq!(result, 321.0); | |
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| extern "C" fn f2(a: f64, b: *const i64) -> f64 { | |
| a + unsafe { *b } as f64 | |
| } | |
| extern "C" fn int_float_int(a: i64, b: f64, c: i64) -> i64 { | |
| a + b as i64 * 10 + c * 100 | |
| } | |
| extern "C" fn float_float_int(a: f64, b: f64, c: i64) -> f64 { | |
| a + b * 10.0 + c as f64 * 100.0 | |
| } | |
| /// Rust port of | |
| /// `rpython/jit/backend/llsupport/test/test_descr.py::test_call_stubs_2`. | |
| #[test] | |
| fn call_stub_f_interleaved_float_ref_preserves_declaration_order() { | |
| let b = [1_i64]; | |
| let result = unsafe { | |
| bh_call_f_dispatch( | |
| f2 as *const () as usize, | |
| &[ArgClass::Float, ArgClass::Int], | |
| &[3.5_f64.to_bits() as i64, b.as_ptr() as i64], | |
| ) | |
| }; | |
| assert_eq!(result, 4.5); | |
| } | |
| #[test] | |
| fn call_stub_i_interleaved_int_float_int_preserves_declaration_order() { | |
| let result = unsafe { | |
| bh_call_i_dispatch( | |
| int_float_int as *const () as usize, | |
| &[ArgClass::Int, ArgClass::Float, ArgClass::Int], | |
| &[1, 2.0_f64.to_bits() as i64, 3], | |
| ) | |
| }; | |
| assert_eq!(result, 321); | |
| } | |
| #[test] | |
| fn call_stub_f_interleaved_float_float_int_preserves_declaration_order() { | |
| let result = unsafe { | |
| bh_call_f_dispatch( | |
| float_float_int as *const () as usize, | |
| &[ArgClass::Float, ArgClass::Float, ArgClass::Int], | |
| &[1.0_f64.to_bits() as i64, 2.0_f64.to_bits() as i64, 3], | |
| ) | |
| }; | |
| assert_eq!(result, 321.0); | |
| } | |
| #[test] | |
| #[should_panic(expected = "unsupported arg class sequence")] | |
| fn call_stub_unsupported_class_sequence_panics() { | |
| let classes = [ | |
| ArgClass::Int, | |
| ArgClass::Int, | |
| ArgClass::Int, | |
| ArgClass::Int, | |
| ArgClass::Int, | |
| ArgClass::Float, | |
| ]; | |
| let args = [0_i64; 6]; | |
| unsafe { bh_call_v_dispatch(int_float_int as *const () as usize, &classes, &args) }; | |
| } | |
| } |
🤖 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 `@majit/majit-backend/src/call_stub.rs` around lines 1377 - 1432, Add a
#[should_panic] test in the existing tests module that invokes the relevant
call-stub dispatcher with a six-argument ArgClass sequence containing exactly
one Float, such as the unsupported shape handled by the fallback arm, and assert
the documented panic message. Reuse the dispatcher and argument representation
used by the nearby call_stub_* tests, ensuring the test verifies that this
sequence panics instead of being dispatched.
| fn arg_classes_from_types( | ||
| args_len: usize, | ||
| arg_types: &[Type], | ||
| ) -> [majit_backend::call_stub::ArgClass; MAX_HOST_CALL_ARITY] { | ||
| assert!( | ||
| args_len <= MAX_HOST_CALL_ARITY, | ||
| "unsupported JitCode typed call arity {args_len} (max {MAX_HOST_CALL_ARITY})" | ||
| ); | ||
| let mut classes = [majit_backend::call_stub::ArgClass::Int; MAX_HOST_CALL_ARITY]; | ||
| for (i, slot) in classes.iter_mut().enumerate().take(args_len) { | ||
| *slot = match arg_types.get(i) { | ||
| Some(Type::Float) => majit_backend::call_stub::ArgClass::Float, | ||
| Some(Type::Int | Type::Ref) | None => majit_backend::call_stub::ArgClass::Int, | ||
| // `descr.py:566-567 TYPE('v')` is `lltype.Void`, which upstream | ||
| // never puts in `arg_classes` for a call it dispatches. | ||
| Some(Type::Void) => panic!("typed call: void argument class at slot {i}"), | ||
| }; | ||
| } | ||
| classes | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert that arg_types covers every positional slot.
arg_types.get(i) returns None when arg_types is shorter than args_len. The slot then becomes ArgClass::Int. If the missing slot is a Float, the dispatcher transmutes the callee to an integer-parameter signature. The value then travels in the integer register file, and the callee reads an unrelated floating-point register. The call returns a wrong result and does not panic.
collect_call_args in majit/majit-backend/src/call_stub.rs (lines 1148-1172) enforces verify_types parity with explicit count assertions. This seam has no equivalent check.
All current callers pass descr.arg_types(), so the lengths should match. Add a debug_assert_eq! so future drift fails loudly instead of corrupting the ABI.
🛡️ Proposed assertion
assert!(
args_len <= MAX_HOST_CALL_ARITY,
"unsupported JitCode typed call arity {args_len} (max {MAX_HOST_CALL_ARITY})"
);
+ // `descr.py:616-620 verify_types` parity: every positional slot must have
+ // a declared class, otherwise a Float silently degrades to an integer
+ // register.
+ debug_assert_eq!(
+ arg_types.len(),
+ args_len,
+ "typed call: arg_types covers {} slots, args has {args_len}",
+ arg_types.len()
+ );
let mut classes = [majit_backend::call_stub::ArgClass::Int; MAX_HOST_CALL_ARITY];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn arg_classes_from_types( | |
| args_len: usize, | |
| arg_types: &[Type], | |
| ) -> [majit_backend::call_stub::ArgClass; MAX_HOST_CALL_ARITY] { | |
| assert!( | |
| args_len <= MAX_HOST_CALL_ARITY, | |
| "unsupported JitCode typed call arity {args_len} (max {MAX_HOST_CALL_ARITY})" | |
| ); | |
| let mut classes = [majit_backend::call_stub::ArgClass::Int; MAX_HOST_CALL_ARITY]; | |
| for (i, slot) in classes.iter_mut().enumerate().take(args_len) { | |
| *slot = match arg_types.get(i) { | |
| Some(Type::Float) => majit_backend::call_stub::ArgClass::Float, | |
| Some(Type::Int | Type::Ref) | None => majit_backend::call_stub::ArgClass::Int, | |
| // `descr.py:566-567 TYPE('v')` is `lltype.Void`, which upstream | |
| // never puts in `arg_classes` for a call it dispatches. | |
| Some(Type::Void) => panic!("typed call: void argument class at slot {i}"), | |
| }; | |
| } | |
| classes | |
| } | |
| fn arg_classes_from_types( | |
| args_len: usize, | |
| arg_types: &[Type], | |
| ) -> [majit_backend::call_stub::ArgClass; MAX_HOST_CALL_ARITY] { | |
| assert!( | |
| args_len <= MAX_HOST_CALL_ARITY, | |
| "unsupported JitCode typed call arity {args_len} (max {MAX_HOST_CALL_ARITY})" | |
| ); | |
| // `descr.py:616-620 verify_types` parity: every positional slot must have | |
| // a declared class, otherwise a Float silently degrades to an integer | |
| // register. | |
| debug_assert_eq!( | |
| arg_types.len(), | |
| args_len, | |
| "typed call: arg_types covers {} slots, args has {args_len}", | |
| arg_types.len() | |
| ); | |
| let mut classes = [majit_backend::call_stub::ArgClass::Int; MAX_HOST_CALL_ARITY]; | |
| for (i, slot) in classes.iter_mut().enumerate().take(args_len) { | |
| *slot = match arg_types.get(i) { | |
| Some(Type::Float) => majit_backend::call_stub::ArgClass::Float, | |
| Some(Type::Int | Type::Ref) | None => majit_backend::call_stub::ArgClass::Int, | |
| // `descr.py:566-567 TYPE('v')` is `lltype.Void`, which upstream | |
| // never puts in `arg_classes` for a call it dispatches. | |
| Some(Type::Void) => panic!("typed call: void argument class at slot {i}"), | |
| }; | |
| } | |
| classes | |
| } |
🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs` around lines 8486 - 8505, Add
a debug_assert_eq! at the start of arg_classes_from_types to require
arg_types.len() == args_len before class generation. Keep the existing arity
assertion and per-slot mapping unchanged.
| extern "C" fn scale_f64(x: f64, k: i64) -> f64 { | ||
| x * k as f64 | ||
| } | ||
|
|
||
| #[test] | ||
| fn interleaved_int_after_float_dispatches_in_declaration_order() { | ||
| let result = call_float_function( | ||
| scale_f64 as *const (), | ||
| &[2.5_f64.to_bits() as i64, 3], | ||
| &[Type::Float, Type::Int], | ||
| ); | ||
| assert_eq!(result, 7.5, "scale_f64(2.5, 3) == 7.5"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn float_after_int_dispatches() { | ||
| extern "C" fn scale_swapped(k: i64, x: f64) -> f64 { | ||
| x * k as f64 | ||
| } | ||
| let result = call_float_function( | ||
| scale_swapped as *const (), | ||
| &[3, 2.5_f64.to_bits() as i64], | ||
| &[Type::Int, Type::Float], | ||
| ); | ||
| assert_eq!(result, 7.5, "scale_swapped(3, 2.5) == 7.5"); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extend the regression tests to the new integer and void helpers.
The two tests cover call_float_function only. This PR also adds call_int_function_typed and call_void_function_typed. Both are new ABI seams, and both are reached from execute_pure_call and execute_residual_call in majit/majit-metainterp/src/executor.rs.
Add one test per helper with an interleaved signature, so an integer-returning or void callee that takes a Float parameter is covered.
💚 Proposed tests
+ extern "C" fn mix_to_int(a: i64, x: f64, b: i64) -> i64 {
+ a + x as i64 * 10 + b * 100
+ }
+
+ #[test]
+ fn typed_int_call_dispatches_interleaved_float_in_declaration_order() {
+ let result = call_int_function_typed(
+ mix_to_int as *const (),
+ &[1, 2.0_f64.to_bits() as i64, 3],
+ &[Type::Int, Type::Float, Type::Int],
+ );
+ assert_eq!(result, 321, "mix_to_int(1, 2.0, 3) == 321");
+ }🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs` around lines 8949 - 8975,
Extend the regression tests near
interleaved_int_after_float_dispatches_in_declaration_order and
float_after_int_dispatches with one test for call_int_function_typed and one for
call_void_function_typed. Use interleaved signatures containing a Float
parameter and assert the integer result or void callee behavior, respectively,
matching the existing call_float_function coverage.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 19027ff). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
1 participant
Follow-up to #990. Three commits on top of
main.The defect
dispatch_arity_body!recovered the callee signature from the bucket counts(n_int, n_float), so a callee declaredfn(f64, i64)was dispatched asfn(i64, f64).collect_call_argsproduced those buckets and never restored thedeclaration order.
That reordering is register-preserving where the integer and floating-point
parameters are drawn from two independent register files in their own relative
order — SysV, AAPCS — and wrong under the Microsoft x64 convention, whose first
four argument slots are positional:
fn(f64, i64)takes xmm0/rdx wherefn(i64, f64)takes rcx/xmm1.x86_64-pc-windows-msvcis a shipped target(
dist-workspace.toml), CI runscargo testandpyre/check.pyonwindows-latest, and pyre's own dynasm emitter already implements the positional
model one layer down (
majit-backend-dynasm/src/x86/assembler.rs, the#[cfg(target_os = "windows")]branch ofbuild_abi_arg_placementsisslot-indexed while the other branch uses two independent counters).
#990 papered over this with an
assert!incall_float_functionrefusing aninteger/ref argument after a float one.
Why the assert was the wrong fix
Upstream buckets too —
executor.py_do_callsplits argboxes intoargs_i/args_r/args_f. The difference is that upstream treats bucketing as atransport format and un-buckets before the machine call:
descr.py:574—args = ", ".join([process(c) for c in self.arg_classes]),and
process(c)(descr.py:553-555) walksarg_classespositionally, pullingthe next item out of the matching bank. The generated stub for
arg_classes == "fr"is literallyfnptr(args_f[0], args_r[0]).descr.py:604-605—ARGS = [TYPE(arg) for arg in self.arg_classes]/FUNC = lltype.FuncType(ARGS, RESULT), declaration order again.test_descr.py::test_call_stubs_2runs exactly the signature the assertrefused:
ARGS = [lltype.Float, lltype.Ptr(ARRAY)], float result, throughcall_stub_f.residual call because of its signature.
And upstream's answer to Win64 is not avoidance:
x86/callbuilder.py:534-556keeps
next_arg_gprandnext_arg_xmmin lockstep underWIN64so slot kclaims rcx-or-xmmk. It can do that because it still holds declaration order
at that point. pyre threw the order away one layer earlier, which is what
broke Win64 and what made the refusal look necessary.
So the assert was a symptom. The deviation is the missing un-bucketing.
The change
a3aa451— replace the bucket-keyed table with a class-sequence-keyed one:ArgClasscarries the two C-ABI register classes ('i','r','L'→Int;'f'→Float, mirroringdescr.py:556-570 TYPE()).dispatch_classes_body!matches on the ordered class slice: every sequenceof length 0..=5 plus the integer-only tail through 16 — 74 arms. Each arm
transmutes to the exact
extern "C" fn(...)with the parameters indeclaration order, so it is correct under SysV, AAPCS and Microsoft x64 alike.
collect_call_argsreturns the ordered class list plus a positional argumentlist, keeping its
verify_typesper-class count assertions(
descr.py:614-620) and its existing'L'/'S'arms.bh_call_{i,f,v}_dispatchtake(classes, args); the dynasm and craneliftbh_call_*overrides andcall_float_functionpass them through.the libffi convergence path.
test_call_stubs_2plus[Int, Float, Int]and[Float, Float, Int], and thecall_float_functionrefusal test is replacedby one asserting the interleaved call now returns the right value.
2b56889—residual_call_float_canonical_via_target_with_effect_infobakestarget.concrete_ptras the funcbox thatexecutor.rs'sType::Floatarm readsback through an
extern "C" fn(..) -> f64ABI. Correct only whileconcrete_ptris the raw callee; a
_concretewrapper has an-> i64signature carryingf64::to_bits. Only the*_float_wrappedpolicies mint that divergence and nocrate declares one, so the invariant is now a
debug_assert_eq!at the bake siterather than a comment resting on that absence.
46ef114— see below.Comment audit
#990's comments were audited assertion by assertion against the code they name.
Six were false and are corrected here:
add_fn_ptratassembler.rs:4617_concretewrapper is emitted by#[jit_module](×2)emit_helper_call_target_fn(majit-macros/src/lib.rs:605-614), reached from the per-helper policy attributes; no crate uses#[jit_module]at all_concrete; that arm wants its owncall_assemblerentry wrapperguard_nonnullahead of a pointer-deref residual callGUARD_NONNULLcomes frompyjitpl.py:558-575 _establish_nullity, i.e. the traced program's own null test; upstream simply never derives that NULLPyFrame.f_backstampsValue::Ref(GcRef(0))into the constant poolf_backref, and it carriesimmutable = false, sois_always_pure()is false and the constant-pool arm is unreachable — the NULL is stamped onto the recorded OpRef viaset_opref_concretef64-returning callee; it read residueAlso narrowed the
executor.py:66-68citation to the result half and citeddescr.py:604-605for the argument half. #990's claim that no interleavedfloat-returning helper exists in the tree was already false —
jit_math_ldexp_raw (f64, i64) -> f64is one — and the paragraph is gone with the restriction.Verification
pyre/check.py --backend dynasm,cranelift,wasm: dynasm 370/370, cranelift370/370, wasm 366/366, all passed. The three reds this branch previously
inherited from
mainare resolved on the current base. The LLBC artefacts forpyre-module/pyre-interpreter/pyre-jitwere stale and were re-extractedfirst, so the run is not measuring a stale build.
cargo test -p majit-backend -p majit-metainterp -p pyre-jit-trace: 1430 + 325 +siblings, 0 failed.
The dispatch table's arm coverage was checked exhaustively rather than by
inspection: all 2^n sequences for n = 0..5 (63) plus all-
Intfor n = 6..16(11) = 74, with no duplicates, no gaps and no arms outside that set.
The rebase onto the post-#990
maindropped the three commits whose contents thesquash merge already carried; the resulting tree hash is byte-identical to the
one the verification above was run against.
— authored by Claude
Summary by CodeRabbit
Bug Fixes
Tests