jit: decline to fold float-result pure calls in the walker; majit-ir clippy allows; record 6 missing jit-stats baselines - #963
Conversation
|
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 (6)
WalkthroughThe change documents intentional Clippy suppressions, clarifies float-call ABI constraints, prevents executor folding for float pure calls, and adds regression coverage for pure-call results and nullable ChangesJIT dispatch behavior
Intentional Clippy annotations
Estimated code review effort: 3 (Moderate) | ~20 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: 75a17be098
ℹ️ 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 call_descr.result_type() == majit_ir::Type::Float { | ||
| return; |
There was a problem hiding this comment.
When a CallPureF result feeds a float comparison and conditional, this early return leaves the call's OpRef without a concrete value; record_float_cmp consequently cannot stamp its result, and fused_goto_if_not_float raises GotoIfNotValueNotConcrete, aborting tracing for that otherwise supported program. RPython's execute_and_record_varargs executes every residual call before recording it, so retain an emitter/ABI discriminator and invoke the matching float-return route rather than skipping execution entirely.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a80e8d6). 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
|
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: 01b0288e24
ℹ️ 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 call_descr.result_type() == majit_ir::Type::Float { | ||
| return; |
There was a problem hiding this comment.
When a runtime-emitted call_pure_float_canonical_via_target survives into a completed trace, this return leaves its CallPureF symbolic even though operand 0 is JitCallTarget::concrete_ptr, whose generated ABI is extern "C" fn(...) -> i64. The native backends subsequently derive an f64 return ABI from the CallDescr and invoke that same pointer as an f64 function, reading the floating-point return register instead of the wrapper's integer result and producing a stale/garbage compiled value. Use the call-target side table to execute the wrapper through the integer-bits route, or record the target's f64-returning trace_ptr, rather than universally deferring float calls to the backend.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
01b0288 to
5638dc3
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/bench/synth/inline_freevar_after_mayforce.wasm.jitstats`:
- Around line 1-5: Regenerate the Wasm jitstats baselines using the complete
descriptor-counter snapshot emitted by the Wasm runner and recorded by check.py.
Add the recorded descr_set_resolved entry to
pyre/bench/synth/inline_freevar_after_mayforce.wasm.jitstats,
pyre/bench/synth/math_log_trig_hot.wasm.jitstats, and
pyre/bench/synth/pypy_dict_primitives_nonbinding.wasm.jitstats, preserving the
existing counters and generated ordering.
🪄 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: b83334aa-5371-4fa6-b645-65161b352c5a
📒 Files selected for processing (24)
majit/majit-ir/src/eval_breaker_word.rsmajit/majit-ir/src/resoperation.rsmajit/majit-ir/src/value.rsmajit/majit-metainterp/src/executor.rspyre/bench/synth/hot_loop_exit_then_class_stmt.cranelift.jitstatspyre/bench/synth/hot_loop_exit_then_class_stmt.dynasm.jitstatspyre/bench/synth/hot_loop_exit_then_class_stmt.wasm.jitstatspyre/bench/synth/inline_freevar_after_mayforce.cranelift.jitstatspyre/bench/synth/inline_freevar_after_mayforce.dynasm.jitstatspyre/bench/synth/inline_freevar_after_mayforce.wasm.jitstatspyre/bench/synth/math_log_trig_hot.cranelift.jitstatspyre/bench/synth/math_log_trig_hot.dynasm.jitstatspyre/bench/synth/math_log_trig_hot.wasm.jitstatspyre/bench/synth/pypy_dict_primitives_nonbinding.cranelift.jitstatspyre/bench/synth/pypy_dict_primitives_nonbinding.dynasm.jitstatspyre/bench/synth/pypy_dict_primitives_nonbinding.wasm.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.cranelift.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.dynasm.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.wasm.jitstatspyre/bench/synth/tuple_unpack_array_backed_hot.cranelift.jitstatspyre/bench/synth/tuple_unpack_array_backed_hot.dynasm.jitstatspyre/bench/synth/tuple_unpack_array_backed_hot.wasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Regenerate all Wasm baselines with the complete descriptor-counter contract.
pyre/pyre-wasm-runner/src/main.rs:652-696 emits descr_set_resolved, and pyre/check.py:981-1033 records the complete snapshot. These baselines omit that counter.
pyre/bench/synth/inline_freevar_after_mayforce.wasm.jitstats#L1-L5: add the recordeddescr_set_resolvedvalue.pyre/bench/synth/math_log_trig_hot.wasm.jitstats#L1-L5: add the recordeddescr_set_resolvedvalue.pyre/bench/synth/pypy_dict_primitives_nonbinding.wasm.jitstats#L1-L5: add the recordeddescr_set_resolvedvalue.
📍 Affects 3 files
pyre/bench/synth/inline_freevar_after_mayforce.wasm.jitstats#L1-L5(this comment)pyre/bench/synth/math_log_trig_hot.wasm.jitstats#L1-L5pyre/bench/synth/pypy_dict_primitives_nonbinding.wasm.jitstats#L1-L5
🤖 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/bench/synth/inline_freevar_after_mayforce.wasm.jitstats` around lines 1
- 5, Regenerate the Wasm jitstats baselines using the complete
descriptor-counter snapshot emitted by the Wasm runner and recorded by check.py.
Add the recorded descr_set_resolved entry to
pyre/bench/synth/inline_freevar_after_mayforce.wasm.jitstats,
pyre/bench/synth/math_log_trig_hot.wasm.jitstats, and
pyre/bench/synth/pypy_dict_primitives_nonbinding.wasm.jitstats, preserving the
existing counters and generated ordering.
`try_fold_pure_call_via_executor` routed every constant-funcbox `CallPure*` through `executor::execute_pure_call`, whose Float arm calls `call_int_function` and reinterprets the integer return register via `f64::from_bits`. That convention holds only for funcboxes baked by the runtime emitter's `*_canonical_via_target` family, which store `JitCallTarget::concrete_ptr` (`jitcode/assembler.rs:3343`) — an `extern "C" fn(...) -> i64` wrapper that pre-packs the `f64`. The codewriter's residual bakes the callee's own address instead, so the `f64` comes back in the floating-point return register. `CallDescr` records the signature, not which emitter produced the pointer, so the arm cannot distinguish them. Return early on `result_type() == Type::Float` and leave the recorded `CallPure*` for the backend, which calls the same pointer with the descr's real signature. `execute_pure_call`'s Float arm keeps the i64-bits convention, now documented as a caller contract. Adds `walker_declines_to_fold_a_float_result_pure_call`, which drives a real `WalkContext` over an `f64`-returning callee plus an `Int` control; without the decline the float box folds to `Float(3.5e-323)` = `f64::from_bits(7)`, the argument still in the integer register. Assisted-by: Claude
`cargo clippy -p majit-ir` failed to compile on three deny-by-default lints, so the crate could not be linted at all: - `ineffective_bit_mask` on the eval-breaker flag-width static assertion; every operand is a constant, so there is no runtime value to compare. - `absurd_extreme_comparisons` on `OpCode::is_final`; `FINAL_FIRST` is `OpCode::Jump as u16` == 0 today, and the bound is kept symbolic to match the sibling classifiers. - `mut_from_ref` on `SharedConstPool::as_mut_vec_for_gc`; the pool is `UnsafeCell`-backed so the GC root walker can mutate through a shared reference, and the existing `# Safety` clause carries the contract. Each site gets a scoped `#[allow]` and a comment stating why. No behavior change; the crate now reports 0 errors and 41 warnings. Assisted-by: Claude
…g refusals `bh_load_global_fn(namespace, w_code, frame, namei)` never reads `namespace` (`call_jit.rs` `let _ = namespace_ptr`): it resolves the globals from the executing frame when that frame owns `w_code`, and from `w_code`'s own live `w_globals` otherwise. The codewriter bakes the operand from `w_code_get_w_globals`, which is `PY_NULL` until a frame for that code object stamps it, so a jitcode built before its callee's first frame carries a concrete NULL there and `walker_abort_if_mayforce_null_ref_arg` aborted the walk. The walker executes residuals eagerly and a non-committed exit replays the walked region from its start, so every residual the walk had already run executed a second time. Exempt arg 0 in both refusals — the walker abort and the executor's decline, which the code requires to stay in step — alongside the existing checked- `PY_NULL` sentinel exemptions for `CallFn`, `CallKw`, `CallFunctionEx`, `RaiseVarargs` and `StoreDeref`. `pickle_terminal_raise_resume` crashed `IndexError: pop from empty list` on all three backends and now prints the interpreter's `checksum = 216`; its `loops_aborted` goes 1 -> 0 and `loops_compiled` 11 -> 33 (dynasm) / 34 (cranelift). `raise_reg_unbound_jitstress` moves `loops_aborted` 10 -> 2 and `loops_compiled` 3 -> 5 on both native backends. Assisted-by: Claude
76555da to
a80e8d6
Compare
1 participant
Two independent changes.
1. Decline to fold float-result pure calls in the walker
try_fold_pure_call_via_executorrouted every constant-funcboxCallPure*through
executor::execute_pure_call, whoseFloatarm callscall_int_functionand reinterprets the integer return register viaf64::from_bits.That convention is a caller contract, not a property of the descr. Two emitters
bake operand 0 of a residual call, with opposite float conventions:
majit-translatecodewriter (LLBC path)f64register*_canonical_via_target(jitcode/assembler.rs:3343)JitCallTarget::concrete_ptri64withf64::to_bitspre-packedBoth land in the same walker handlers and dispatch on
descr.result_type()alone.
CallDescrrecords the signature, not which emitter produced thepointer, so no descr-based dispatch can be right for both — feeding a
codewriter funcbox to the
i64-bits arm reads whatever was left in the integerreturn register.
The fix returns early on
result_type() == Type::Floatand leaves the recordedCallPure*for the backend, which calls the same pointer with the descr's realsignature (
majit-backend-wasm/src/codegen.rs:909 residual_call_float_sigbuilds an
f64-returningcall_indirectfrom it).execute_pure_call'sFloatarm keeps thei64-bits convention, now documented as a callercontract.
Adding a
bh_call_finstead would only flip which channel is broken. Thediscriminator does exist in the data —
JitCode::call_descr_to_call_target(
jitcode/mod.rs:367) maps a descr slot back to itsJitCallTarget— butnothing in
pyre/reads it today, so folding these is left to a future change.Evidence. New regression test
walker_declines_to_fold_a_float_result_pure_calldrives a real
WalkContextover anf64-returning callee plus anIntcontrol. Without the decline the float box folds to
Float(3.5e-323)=f64::from_bits(7)— the argument still sitting in the integer register —while the
CallPureIsibling in the same fixture folds to42, so the fixtureis not the problem. A standalone probe against
jit_bigint_to_f64_or_infmismatches 7/7 (
std::hint::black_boxis required, or LLVM sees through thetransmute and re-emits a correctly-ABI'd direct call, which is why the
pre-existing
float_result_routes_through_call_int_function_with_bits_packingtest passed).
Latent, not live. Instrumenting the fold for
result_type() == Floatandsweeping all ~1400 files of
pyre/bench+pyre/bench/synth(check.py'scorpus) found zero occurrences.
2. Allow three deny-by-default clippy lints in
majit-ircargo clippy -p majit-irfailed to compile on three deny-by-default lints, sothe crate could not be linted at all. Each is a false positive against
deliberate structure and gets a scoped
#[allow]plus a comment stating why:ineffective_bit_maskon the eval-breaker flag-width static assertion — everyoperand is a constant, so there is no runtime value to compare.
absurd_extreme_comparisonsonOpCode::is_final—FINAL_FIRSTisOpCode::Jump as u16== 0 today; the bound is kept symbolic to match thesibling classifiers and to stay correct if the category stops leading the enum.
mut_from_refonSharedConstPool::as_mut_vec_for_gc— the pool isUnsafeCell-backed so the GC root walker can mutate through a sharedreference, and the existing
# Safetyclause carries the contract.No behavior change; the crate now reports 0 errors and 41 warnings.
Verification
python3 pyre/check.py --backend dynasm,cranelift: 14 failed / 351 passed perbackend, byte-identical failure set to the clean-HEAD control and to a
repeat run. All 14 are pre-existing (6 missing jit-stats baselines from jit: give synthetic tuple and Result shells their own low-level layouts, and keep freevar cells across inline and resume #934,
8 jit-stats regressions incl. the
exception_reraise_tb_depth_jitstressfamily).
cargo test -p pyre-jit-trace -p majit-metainterp --features dynasm: green.cargo test -p majit-ir: 307 passed.cargo clippy -p majit-ir --no-deps: 0 errors.cargo fmt --all --check: clean.3. Record the six missing synthetic jit-stats baselines
hot_loop_exit_then_class_stmt(#890) and five fixtures from #934/#948 wereadded without a committed
.jitstats, so check.py failed them on both backendswith "no committed jit-stats baseline". Recorded one fixture at a time with
--snapshot --synthetic-pattern <name>.py, so no existing baseline isrewritten; 350 synth scripts now have 350 dynasm and 350 cranelift baselines.
Two record a weak floor worth revisiting:
pypy_dict_primitives_nonbindingcompiles no loop at all (
loops_compiled=0), andraise_reg_unbound_jitstressenters at
loops_aborted=10.CI status — the remaining 8 failures are pre-existing on
mainmainatf3bddb31f28failspyre/check.pyon all three platforms withexactly the same 14 fixtures and the same numbers as this branch did before
commit 3 (compare run 30707884899 against 30713388334). This PR introduces no
check.py regression; the 6 baseline recordings above take it from 14 to 8
failed / 357 passed per backend.
The 8 that remain are jit-stats floor regressions that entered
mainbetween#947 (which recorded the baselines) and the current tip. Measured root split —
one build with an env-gated early
return trueinoptimizeopt/heap.rs quasiimmut_field_still_valid, all 8 fixtures re-run bothways:
exception_reraise_tb_depth_jitstressSo exactly one is #956's quasi-immutable revalidation. Reverting #956 is not
the fix: PyPy aborts 2100× on this same script (
abort: force quasi-immut), sofiring here is faithful — what pyre lacks is upstream's primary mechanism,
opimpl_jit_force_quasi_immut(pyjitpl.py:1104-1118), which aborts early atthe write instead of letting every event fall through to the optimizer
backstop.
ABORT_FORCE_QUASIIMMUTis a dead counter in pyre today(
pyjitpl.rs:15634, no site raises it). That port is a separate slice.The other 7 (
closure_freevar_branch_resume,exception_args_virtual,exception_multi_handler_warmup,list_length_hint_validate,sre_pattern_methods,sre_wasm_min,sre_wasm_min1) have a different,still-unbisected root in the same window.
— authored by Claude
Summary by CodeRabbit
Bug Fixes
Tests