Repository navigation
jit: dispatch float residual calls through the float return register - #990
Conversation
WalkthroughThe change adds ABI-aware floating-point call dispatch. Pure and residual float-returning calls now execute through the floating-point path and can fold concrete results. Tests cover mixed argument classes, floating-point registers, and ChangesFloating-point call dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Executor
participant call_float_function
participant HostTrampoline
participant bh_call_f_dispatch
Executor->>call_float_function: typed arguments and raw float bits
alt host trampoline available
call_float_function->>HostTrampoline: dispatch classified arguments
else host trampoline unavailable
call_float_function->>bh_call_f_dispatch: dispatch classified arguments
end
call_float_function-->>Executor: f64 return value
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 4fb34e2). 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
|
![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: 83a5197618
ℹ️ 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".
| assert!( | ||
| float_args.is_empty(), | ||
| "call_float_function: interleaved int/float parameters are \ | ||
| outside `bh_call_f_dispatch`'s `fn(I.., F..)` arity table" | ||
| ); |
There was a problem hiding this comment.
On native targets, a CallPureF/CallF descriptor such as [Float, Int] reaches this assertion and panics instead of executing; this is a valid signature already used by jit_math_ldexp_raw(f64, i64) -> f64 and its descriptor in specialize.rs:6092-6096. The existing bh_call_f_by_classes path accepts interleaving by separating the ABI register banks, so this new executor path should do likewise (or retain the pure-call decline for unsupported layouts) rather than aborting tracing.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
Operand 0's float ABI is set by how its `JitCallTarget` was registered, not by which emitter ran: `add_fn_ptr(ptr)` is `add_call_target(ptr, ptr)` (`jitcode/assembler.rs:4617`) so `concrete_ptr` is the raw callee address, while the policy-macro lowering registers `#[jit_module]`'s `_concrete` i64-packing wrapper; the LLBC codewriter mints no `JitCallTarget` at all. `residual_call_float_canonical_via_target_with_effect_info` bakes `target.concrete_ptr` for all of them. Also records that `pyjitpl/dispatch.rs:6539` calls the same `concrete_ptr` through `bh_call_f_by_classes` as a real f64 return, the opposite of what `execute_pure_call`'s Float arm assumes, and fixes two stale line citations (`jitcode/assembler.rs:3343`, `codegen.rs:909`). Comments only. Assisted-by: Claude
`execute_pure_call` and `execute_residual_call` routed a `Type::Float` result through `call_int_function`, an `extern "C" fn(..) -> i64` transmute, so the result was read from the integer return register and float arguments were passed in the integer register file. Adds `call_float_function(func_ptr, args, arg_types)`, the `bh_call_f` sibling of `call_int_function` (`executor.py:66-68`, `llmodel.py:828`): it splits the positional args into the int/float banks `bh_call_f_dispatch` expects and returns the `f64`. Float slots carry raw `f64::to_bits` in both directions. Interleaved int/float parameters are outside the arity table and assert rather than shuffle registers. Both walker seams now use it, replacing the `result_type() == Float` decline in `try_fold_pure_call_via_executor`. The decline covered only the pure path; `try_execute_residual_call_via_executor` admits `CallF` / `CallLoopinvariantF` / `CallMayForceF` with no such guard and stamped `Value::Float(f64::from_bits(..))` from the same integer read. Extending the decline there was not available: its `Declined` arm runs `fbw_abort_nested_unjournaled_residual` + `fbw_mark_unjournaled_effect`. The funcbox reaching these seams is the callee's own address on every live path — `add_fn_ptr(ptr)` is `add_call_target(ptr, ptr)`, which is what pyre's codewriter registers, and the LLBC path bakes a plain `ConstInt` fnaddr with no `JitCallTarget`. The `f64::to_bits`-packing `_concrete` wrapper reaches a residual call only through the `*_float_wrapped` call policies, which no crate declares. The CALL_ASSEMBLER arms that do want that wrapper keep `call_int_function`. `execute_varargs`'s Float arm is unchanged; its caller set includes the i64-returning portal runner. Tests: the two executor float tests now use real `extern "C" fn(f64) -> f64` and `fn(i64) -> f64` callees instead of hand-packed i64 helpers, and the walker test asserts the fold produces 3.5 rather than asserting the decline; an integer-register read would stamp the argument 7. check.py dynasm/cranelift/wasm: 2 failed each, byte-identical to the pre-change run (all three inherited from main). Assisted-by: Claude
…old refusal `dispatch_arity_body!` recovers a callee signature from the bucketed (int, float) arity, which reorders an interleaved signature. Record that this preserves register assignment on SysV/AAPCS, where the two register files are filled in independent relative order, but not on the Microsoft x64 convention, where the first four argument slots are positional. `collect_call_args` states that it does not reject interleaving and why. `call_float_function`: restate the panic's rationale in those terms, note that libffi is the recorded convergence path, and record that `descr.py`'s 'L'/'S' argument classes cannot reach the `arg_types()` seam because `CallDescr::arg_classes` maps `Type` onto 'i'/'r'/'f'/'v'. `try_fold_pure_call_via_executor`: name the upstream test being deviated from (`record_result_of_call_pure` folds on all-Const, admitting `ConstPtr(NULL)`) and where pyre's concrete NULL comes from that upstream would still hold symbolically. Assisted-by: Claude
83a5197 to
4fb34e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb34e214b
ℹ️ 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".
| majit_ir::Type::Float => { | ||
| crate::pyjitpl::call_float_function(func_ptr, args, descr.arg_types()).to_bits() as i64 |
There was a problem hiding this comment.
When a native dynasm/cranelift trace uses an explicit float-wrapped policy such as elidable_float_cannot_raise_wrapped, the macro registers distinct trace and concrete targets, and residual_call_float_canonical_via_target_with_effect_info bakes target.concrete_ptr into the funcbox; that generated _concrete wrapper returns i64 containing f64::to_bits. Calling it here through an extern "C" fn(...) -> f64 ABI reads the floating-point return register instead, so pure folding can stamp an arbitrary float into the trace. Preserve the target discriminator/use the trace target, or decline folding when the funcbox is the integer-return wrapper.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| unsafe { | ||
| majit_backend::call_stub::bh_call_f_dispatch(func_ptr as usize, &int_args, &float_args) |
There was a problem hiding this comment.
For a valid constant CallPureF signature within MAX_HOST_CALL_ARITY but outside dispatch_arity_body!—for example five Float arguments—this newly reachable call falls into the backend catch-all and panics while tracing; before this change, every float-result pure call remained symbolic for the backend to execute. Either check that the (int, float) counts are represented before invoking this dispatcher or implement the general per-signature dispatch, rather than turning an otherwise valid residual call into a tracer panic.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
![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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 8474-8501: Add a focused #[should_panic] test for
call_float_function that passes interleaved argument types such as [Type::Float,
Type::Int] and matching arguments, exercising the assertion that rejects
integer/ref parameters after floating-point parameters. Keep the test scoped to
validating this panic contract.
🪄 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: 9b054696-097b-4af4-8e87-0db81d7d9cee
📒 Files selected for processing (6)
majit/majit-backend/src/call_stub.rsmajit/majit-metainterp/src/executor.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
| pub fn call_float_function(func_ptr: *const (), args: &[i64], arg_types: &[Type]) -> f64 { | ||
| // Where a backend cannot build a `call_indirect` whose type matches the | ||
| // callee's real signature (wasm32), route through the host trampoline, | ||
| // which reflects the signature and returns an f64 result as its raw bits. | ||
| if let Some(hook) = majit_backend::call_stub::residual_host_call() { | ||
| return f64::from_bits(hook(func_ptr as usize, args) as u64); | ||
| } | ||
| let mut int_args: Vec<i64> = Vec::with_capacity(args.len()); | ||
| let mut float_args: Vec<f64> = Vec::new(); | ||
| for (i, &a) in args.iter().enumerate() { | ||
| match arg_types.get(i) { | ||
| Some(Type::Float) => float_args.push(f64::from_bits(a as u64)), | ||
| _ => { | ||
| assert!( | ||
| float_args.is_empty(), | ||
| "call_float_function: an integer/ref parameter after a float \ | ||
| one cannot be bucketed into `bh_call_f_dispatch`'s \ | ||
| `fn(I.., F..)` signature without changing which register \ | ||
| each argument lands in on a positional-slot ABI" | ||
| ); | ||
| int_args.push(a); | ||
| } | ||
| } | ||
| } | ||
| unsafe { | ||
| majit_backend::call_stub::bh_call_f_dispatch(func_ptr as usize, &int_args, &float_args) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for the interleaved-signature rejection.
The assert! at Line 8487 is the safety mechanism that prevents silent register corruption for interleaved int/float signatures. No test in the provided diff exercises this panic path. Add a #[should_panic] test that calls call_float_function with arg_types such as [Type::Float, Type::Int] to lock in this contract.
🤖 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 8474 - 8501, Add
a focused #[should_panic] test for call_float_function that passes interleaved
argument types such as [Type::Float, Type::Int] and matching arguments,
exercising the assertion that rejects integer/ref parameters after
floating-point parameters. Keep the test scoped to validating this panic
contract.
Follow-up to #963, which landed a
result_type() == Floatdecline intry_fold_pure_call_via_executorrather than fixing the ABI underneath it.This removes the decline by fixing the ABI.
The defect
execute_pure_callandexecute_residual_callrouted aType::Floatresultthrough
call_int_function, anextern "C" fn(..) -> i64transmute. The C ABIreturns
f64in xmm0 / d0, so the result was read from the integer returnregister — which still held an argument. Float arguments were wrong in the
same way, passed in the integer register file rather than the FP one.
executor.py:66-68dispatches FLOAT throughcpu.bh_call_f, whose backendimplementation calls a per-descr stub built from the descr's own signature
(
llmodel.py:828→descr.py:598-612 create_call_stub). pyre has alreadyported that as
majit_backend::call_stub::bh_call_f_by_classes/bh_call_f_dispatch, and two of the three residual-call consumers already useit on this exact operand — the blackhole's
bhimpl_residual_call_irf_fand themetainterp's own
BC_RESIDUAL_CALL_IRF_Fdispatcher.executor.rswas the loneoutlier.
Why the decline could not just be extended
The decline covered only the pure path. The non-pure sibling
try_execute_residual_call_via_executoradmitsCallF/CallLoopinvariantF/CallMayForceFwith no such guard, runs the same integer read viaexecute_residual_call, and stampsValue::Float(f64::from_bits(..)).Copying the guard there is not available: its
Declinedarm runsfbw_abort_nested_unjournaled_residual+fbw_mark_unjournaled_effect, so adecline trades a wrong value for an unjournaled walk whose region the caller
then replays. Fixing the ABI closes both seams; declining can only ever close
one.
The change
call_float_function(func_ptr, args, arg_types) -> f64— thebh_call_fsibling of
call_int_function. It splits the positional args into the int/floatbanks
bh_call_f_dispatchexpects, keeps the wasm32 host-trampoline path, andreturns the
f64. Float slots carry rawf64::to_bitsin both directions(
longlongfloat-storage parity). Both walker seams use it; the decline isdeleted.
bh_call_f_dispatch's arity table isfn(I × ints, F × floats)— allinteger/ref parameters precede all floats. Interleaved signatures assert rather
than shuffle registers. Every float-returning helper in the tree is
non-interleaved:
jit_bigint_to_f64_or_inf(ref) -> f64,jit_float_abs(f64) -> f64,jit_float_fmod(f64, f64) -> f64.Why this is safe: operand 0's ABI is set by REGISTRATION, not by emitter
#963's comment said the split was "codewriter vs the
*_canonical_via_targetfamily". That is wrong —
residual_call_float_canonical_via_target_with_effect_infobakestarget.concrete_ptrfor every producer. The discriminator is theJitCallTargetregistration site:concrete_ptrisadd_fn_ptr(ptr)=add_call_target(ptr, ptr)add_call_target_with_save_err(trace, concrete)#[jit_module]'s_concretewrapper,extern "C" fn(i64,..) -> i64withf64::to_bitsConstInt(fnaddr_for_target)— noJitCallTargetat allpyre's own codewriter registers via
add_fn_ptr/add_fn_ptr_with_slot, soits
concrete_ptris the raw address.The i64-packing wrapper reaches a residual call only through the explicit
*_float_wrappedcall policies — the macro'sHelperCallKind::Floatarm emitsan
UNSUPPORTEDpolicy byte for every attribute otherwise. No crate declaresone:
So every live float residual-call funcbox is a raw callee address. This is a
compile-time fact, so it needed no instrumented build.
The first commit corrects those comments in place, including two stale line
citations (
jitcode/assembler.rs:3343,codegen.rs:909).Deliberately unchanged
execute_varargs's Float arm keepscall_int_function— its caller setincludes
do_recursive_call's portal runner (bh_portal_runner -> i64), andthat convention was not verified here.
CALL_ASSEMBLERarms (pyjitpl/dispatch.rs,blackhole.rs) genuinely dowant the i64 wrapper and are a separate opcode family.
Verification
cargo test -p majit-metainterp -p pyre-jit-trace: 1428 + 322 + siblings, 0failed.
pyre/check.py --backend dynasm,cranelift,wasm: 2 failed each, byte-identicalto the pre-change run on the same base —
exception_args_virtual(all three),list_length_hint_validate(dynasm/cranelift),pickle_terminal_raise_resume(wasm). All three are inherited from
main; the last was attributed by buildingorigin/mainitself with none of these commits and measuring the sameloops_aborted = 59.The corpus is neutral because no synth fixture drives a float residual call
through the walker executor (
math_sqrt_hot/math_log_trig_hotshort-circuitin the walker specializers). The evidence is in the unit tests instead:
halve_f64_for_walker_test(7)to3.5; aninteger-register read stamps the argument
7;extern "C" fn(f64) -> f64andfn(i64) -> f64callees, replacing hand-packed i64 helpers that had pinnedthe broken convention.
Codex parity review disposition
Section 1: none. Sections 2–4 were checked against the cited upstream lines
before acting; both section-2 findings are reclassified, and the section-3
finding is a structural adaptation. The third commit records each reason in
code rather than changing behaviour.
2.1 — the interleaved-signature
assert!vscreate_call_stub's arbitraryarg_classesorder. Reclassified.bh_call_*_dispatchrecovers the calleesignature from the bucketed
(int, float)arity, sofn(f64, i64)would bedispatched as
fn(i64, f64). That is register-preserving under SysV and AAPCS,which fill the integer and floating-point files in independent relative order,
and wrong under the Microsoft x64 convention, whose first four argument slots
are positional —
fn(f64, i64)takes xmm0/rdx wherefn(i64, f64)takesrcx/xmm1. Upstream needs no such restriction because
create_call_stubgenerates a stub carrying the descr's real signature; the convergence path for
pyre is the libffi dispatch already named in
dispatch_arity_body!'s catch-allarm. The assertion is kept and its rationale restated in those terms.
Reviewing this did surface a real gap Codex did not flag:
collect_call_argsperforms the same reordering on the bank-fed path (
bh_call_{i,f,v}_by_classes)with no check at all. Not changed here — a blanket rejection would panic on
calls that are ABI-correct under SysV/AAPCS on the blackhole and backend hot
paths, and the correct fix is the same libffi dispatch. The gap is now stated
at the function.
2.2 —
Type::Floattreated asf64, losing'L'(SignedLongLong) and'S'(SingleFloat). Reclassified as unreachable at this seam. The bank-fedcollect_call_argsdoes carry both arms ('L'→ integer register file,'S'→ explicit panic).
call_float_functionis fed bydescr.arg_types(), andCallDescr::arg_classesmapsTypeonto'i'/'r'/'f'/'v'exhaustively,so a
Type::Floatslot is always class'f'. Documented at the seam so thatteaching
Typethose classes extends both sites.3 — the NULL-Ref fold refusal vs
record_result_of_call_pure. Structuraladaptation, not pre-existing debt. Upstream's weaker test ("every argbox is a
Const", admittingConstPtr(NULL)) is reached only after the call has alreadyexecuted for real, on constants the interpreter itself produced. Pyre's walker
folds from a different source:
getfield_gc_rpropagates pointer-valued fieldreads as concrete whenever the parent struct is concrete-known, so a top-level
frame stamps
Value::Ref(GcRef(0))where upstream still holds a symbolic boxguarded by the
guard_nonnullits optimizer inserts ahead of a pointer-derefresidual call. Executing
helper(NULL)before that guard exists dereferencesNULL. The comment now names the upstream test and where the divergence in
constant provenance comes from.
4 — both items accepted as documented adaptations. The
to_bits() as i64result is the
longlongfloat-storage equivalent; the shared dispatch tablestands in for translation-time per-descr stub generation, and 2.2 above is the
descriptor-class semantics that entry asks it to retain.
cargo test -p majit-metainterp -p majit-backend -p pyre-jit-trace: 1428 + 325message only, so the measured corpus results above still stand.
— authored by Claude
Summary by CodeRabbit
New Features
Bug Fixes
Documentation