fix(codegen): remove the unproven i64 function specialization (#7238) - #7242
Merged
Conversation
`emit_i64_specializations` re-emitted a whole `number`-typed function body in
i64 arithmetic and wrapped it in an f64 shim that `fptosi`d every argument and
`sitofp`d the result. Two independent halves of its contract were unchecked:
* overflow — i64 add/sub/mul are exact, JS rounds to the nearest double at
every operator, so the two agree only while every intermediate satisfies
|v| <= 2^53;
* argument truncation — a `number` parameter is a double, and the wrapper's
`fptosi double %arg to i64` truncated a fractional one on entry.
Neither is statically provable for the self-recursive bodies the pass existed
to serve: a parameter fed by its own recursive-call argument has no bound, so
#7237's `i32_chain_magnitude_bits` has no bounded leaf to measure from. Even
the motivating example diverges (`fib(79)` crosses 2^53). The pass also
suppressed the sound replacements — `typed_f64`/`typed_i32`/`typed_i1` clones
and the Phase-2 specialized ABI were all retained minus the i64-specialized
set — so removing it hands those functions to a specializer that proves what
this one assumed.
Claimed 2 of the 30 `benchmarks/suite/` programs (`05_fibonacci`, `14_closure`).
Fixes #7238.
📝 WalkthroughWalkthroughThe PR removes the unsound whole-function i64 specialization path. Normal double lowering remains active, while specialized-ABI and typed-ABI selection use updated exclusions. Tests cover overflow, fractional arguments, recursion, and exact double code generation. ChangesNumber exactness codegen
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/perry-codegen/tests/native_proof_regressions.rs (1)
10039-10048: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the fixture to match the new test intent.
The test no longer covers an i64-specialization collision, but it still builds
typed_f64_i64_specialized_collision_module(). A name such asnumber_add_moduledescribes what the fixture now provides. This is optional.🤖 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 `@crates/perry-codegen/tests/native_proof_regressions.rs` around lines 10039 - 10048, Rename the fixture used by number_add_function_takes_the_typed_f64_clone_not_an_i64_body from typed_f64_i64_specialized_collision_module to a name describing the number-add module, such as number_add_module, and update its definition and all references accordingly.crates/perry-codegen/src/codegen/mod.rs (1)
2156-2160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the matching comment on the
spec_abi_functionsinitializer.This comment now correctly states that plan selection runs after the typed-ABI clone sets. The initializer of
spec_abi_functionsstill carries the old wording, which names the removed i64-specialization pass as the ordering constraint. Align both so the ordering contract has one description.♻️ Proposed comment fix at the initializer
- // Phase 2 spec-ABI plans are selected AFTER the i64-specialization - // pass (mutual exclusion), below; start empty here. + // Phase 2 spec-ABI plans are selected AFTER the typed_abi clone sets + // (mutual exclusion), below; start empty here. spec_abi_functions: std::collections::HashMap::new(),🤖 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 `@crates/perry-codegen/src/codegen/mod.rs` around lines 2156 - 2160, Update the comment attached to the spec_abi_functions initializer to describe ordering in terms of the typed_abi clone sets, matching the Representation-selection Phase 2 comment. Remove the outdated reference to the removed i64-specialization pass while preserving the existing scope and bounded-selection details.crates/perry-codegen/tests/i64_spec_ternary_recursion.rs (1)
207-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive double-body assertion here too.
Both assertions are negative. After the pass removal neither
halfDown_i64nor an entryfptosican be emitted for any lowering of this shape, so the test cannot distinguish a correct double body from a broken one. The sibling test at Line 172 already slices the public body and assertsbr i1pluscall double. Apply the same positive checks.🤖 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 `@crates/perry-codegen/tests/i64_spec_ternary_recursion.rs` around lines 207 - 214, Add positive assertions in the i64 ternary recursion test alongside the existing checks, verifying the public double body contains the expected br i1 and call double instructions. Follow the sibling test’s body-slicing approach rather than relying only on absence of halfDown_i64 or entry fptosi.crates/perry-codegen/src/codegen/number_exactness_tests.rs (1)
339-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion so this test cannot pass vacuously.
The only assertion is that
halfDown_i64is absent. After the pass removal that symbol can never be emitted for any input, so the test holds even ifhalfDownwere lowered incorrectly. The doc comment above claims coverage for "the whole class of reasons". Assert that the public body keeps the double path, as the sibling tests do.♻️ Proposed strengthening
let ir = emitted_ir(vec![f]); assert!(!ir.contains("halfDown_i64"), "{NO_I64_BODY}:\n{ir}"); + let body = function_ir(&ir, "`@perry_fn_number_exactness_ts__halfDown`(") + .unwrap_or_else(|| panic!("public f64 body for `halfDown` must be emitted:\n{ir}")); + assert!( + body.contains("call double `@perry_fn_number_exactness_ts__`"), + "`halfDown` must recurse through a double-typed body:\n{body}" + );🤖 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 `@crates/perry-codegen/src/codegen/number_exactness_tests.rs` around lines 339 - 340, Add a positive assertion in the test around emitted_ir so it verifies the public halfDown body retains the expected double-path lowering, matching the sibling tests. Keep the existing absence assertion for halfDown_i64, ensuring the test checks both that the removed i64 helper is not emitted and that halfDown itself is lowered correctly.test-files/test_gap_7238_i64_specialization_exactness.ts (1)
67-68: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
fib(40)dominates the runtime of this parity case.
fib(40)is roughly 1.6e9 calls and, per the changelog, takes ~555 ms after the change.fib(25)on Line 67 already covers the exact-chain shape. If the gap suite runs on every PR, consider lowering the second bound (for examplefib(32)) to keep the case cheap. Keepfib(40)if the crossing of 2^53 by intermediate values is the point being asserted.🤖 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 `@test-files/test_gap_7238_i64_specialization_exactness.ts` around lines 67 - 68, Reduce the second Fibonacci workload in the parity case by changing the fib(40) call near the existing fib(25) check to a lower bound such as fib(32), since fib(25) already covers the exact-chain shape. Preserve fib(40) only if this test explicitly asserts intermediate values crossing 2^53.
🤖 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 `@crates/perry-codegen/src/codegen/number_exactness_tests.rs`:
- Around line 268-271: Update the assertion in the number exactness test to
check only that the IR does not contain “define i64 `@perry_fn_number_exactness`”.
Remove the unrelated “alwaysinline” disjunction while preserving the existing
failure message and IR context.
- Around line 286-289: Update the argument-truncation assertions in
crates/perry-codegen/src/codegen/number_exactness_tests.rs:286-289,
crates/perry-codegen/tests/i64_spec_ternary_recursion.rs:184-187 and 211-214,
and crates/perry-codegen/tests/native_proof_regressions.rs:10049-10052 to match
each function’s emitted SSA parameter names, such as %arg1, or inspect the
truncation opcode within the sliced public body. Ensure entry truncation using
the actual emitted arguments cannot satisfy these checks.
---
Nitpick comments:
In `@crates/perry-codegen/src/codegen/mod.rs`:
- Around line 2156-2160: Update the comment attached to the spec_abi_functions
initializer to describe ordering in terms of the typed_abi clone sets, matching
the Representation-selection Phase 2 comment. Remove the outdated reference to
the removed i64-specialization pass while preserving the existing scope and
bounded-selection details.
In `@crates/perry-codegen/src/codegen/number_exactness_tests.rs`:
- Around line 339-340: Add a positive assertion in the test around emitted_ir so
it verifies the public halfDown body retains the expected double-path lowering,
matching the sibling tests. Keep the existing absence assertion for
halfDown_i64, ensuring the test checks both that the removed i64 helper is not
emitted and that halfDown itself is lowered correctly.
In `@crates/perry-codegen/tests/i64_spec_ternary_recursion.rs`:
- Around line 207-214: Add positive assertions in the i64 ternary recursion test
alongside the existing checks, verifying the public double body contains the
expected br i1 and call double instructions. Follow the sibling test’s
body-slicing approach rather than relying only on absence of halfDown_i64 or
entry fptosi.
In `@crates/perry-codegen/tests/native_proof_regressions.rs`:
- Around line 10039-10048: Rename the fixture used by
number_add_function_takes_the_typed_f64_clone_not_an_i64_body from
typed_f64_i64_specialized_collision_module to a name describing the number-add
module, such as number_add_module, and update its definition and all references
accordingly.
In `@test-files/test_gap_7238_i64_specialization_exactness.ts`:
- Around line 67-68: Reduce the second Fibonacci workload in the parity case by
changing the fib(40) call near the existing fib(25) check to a lower bound such
as fib(32), since fib(25) already covers the exact-chain shape. Preserve fib(40)
only if this test explicitly asserts intermediate values crossing 2^53.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50c85ecd-fad5-44a9-ba07-17db3e701f57
📒 Files selected for processing (13)
changelog.d/7242-i64-spec-exactness.mdcrates/perry-codegen/src/codegen/i64_spec.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/number_exactness_tests.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/codegen/typed_abi.rscrates/perry-codegen/src/codegen/typed_abi_opt_report.rscrates/perry-codegen/src/collectors/clamp_detect.rscrates/perry-codegen/src/collectors/i64_emit.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/tests/i64_spec_ternary_recursion.rscrates/perry-codegen/tests/native_proof_regressions.rstest-files/test_gap_7238_i64_specialization_exactness.ts
💤 Files with no reviewable changes (4)
- crates/perry-codegen/src/codegen/typed_abi_opt_report.rs
- crates/perry-codegen/src/codegen/typed_abi.rs
- crates/perry-codegen/src/collectors/i64_emit.rs
- crates/perry-codegen/src/codegen/i64_spec.rs
added 2 commits
August 2, 2026 10:56
CodeRabbit review on #7242. Three assertions matched the literal `fptosi double %arg`, which couples them to how parameters happen to be named in the emitted IR. Match on the opcode alone, scoped to the public function body, so a rename cannot turn them vacuous — none of these fixtures has another reason to narrow a double to an integer. Also renames the `native_proof_regressions` fixture to `number_add_module`, since it no longer describes an i64-specialization collision.
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7238.
The bug
f8f1e7188grow(40, 1)18236498188585394000-210245885124158400frac(3, 0.5)40Two mechanisms, both in the same pass
emit_i64_specializationsre-emitted a wholenumber-typed function body ini64 arithmetic and wrapped it in an f64 shim.
1. Overflow —
crates/perry-codegen/src/collectors/i64_emit.rs:148-158(
i64_val'sBinaryarm →add/sub/mul i64), admitted bycrates/perry-codegen/src/collectors/clamp_detect.rs:278(i64s_expr).Nothing bounded any intermediate. i64 arithmetic is exact; JS rounds to the
nearest double at every operator, so the two agree only while each
intermediate satisfies
|v| <= 2^53. Between 2^53 and 2^63 the answers merelydiffer; past 2^63 the chain wraps and the sign flips.
2. Argument truncation —
crates/perry-codegen/src/codegen/i64_spec.rs:73(
blk.fptosi(DOUBLE, &format!("%arg{}", p.id), I64)). Anumberparameter isa double.
fptositruncated a fractional argument on entry, and thesitofpat line 79 could not represent a fractional result on the way out.
The two are independent:
frac(3, 0.5)never leaves the exact range, andgrow(40, 1)is called with exact integers.The invariant, and why #7237's machinery does not transfer
#7237 stated it for i32:
At i64 the same bound applies, plus an argument-domain proof the i32 path
did not need:
i32_chain_magnitude_bitsmeasures bounded leaves — an i32slot, an integer literal's own width, a masked or shifted value, a
const'sliteral magnitude — and composes them (
Add/Sub+1 bit,Mulsums widths).Here there is no bounded leaf. The leaves are
numberparameters, and thepass is only observable through two paths, both of which defeat the
measurement:
unbounded by construction.
grow's accumulator, andfib's return value:fib(79)already crosses 2^53, so the pass's own motivating examplediverges from Node.
wrapper matters, but a call through a function value is not.
apply2(mulAdd, 1.5, 2.5)printed3, not4.75.So the admission set under static proof is empty, and repairing it would need
a runtime guard plus a deopt path back to an f64 body — which the pass
deliberately did not emit (
i64_specializedsuppressed it). Per CLAUDE.md'skill-policy the pass is removed rather than left as an unprovable mode, which
is option (b) in the issue.
Removal also unblocks the sound specializers the pass was displacing:
typed_f64_functions/typed_i32_functions/typed_i1_functionsand thePhase-2 specialized ABI were all retained minus the i64-specialized set
(
codegen/mod.rs:2213-2224,:2298).14_closure'scomputenow takes a__typed_f64clone behind a guarded public wrapper — a call-site-provenspecialization instead of an assumed one.
Red then green
test-files/test_gap_7238_i64_specialization_exactness.ts— the issue's twoshapes, an overflow that lands between 2^53 and 2^63, fractional arguments
(positive, negative, and one that truncates to zero), the 2^53 boundary from
both sides (
2^52/2^53/2^54,+1across it, and the negative mirror), anon-recursive function reached through a function value and through a
higher-order call, comparisons on fractional arguments, and the chains that
must stay exact (
fib,fact,sumTo,tri).main: 11 lines diverge fromnode --experimental-strip-types26.5.1 — 5 from overflow, 6 from argument truncation.
test-parity/gap_snapshot.json(which lists only non-passing tests).Evidence
IR, all 30
benchmarks/suite/programs, byte-for-byte vsmain: 28unchanged. The two that move are exactly the two that carried a
specialization:
05_fibonacci—fibloses its_i64body andfptosishim for an exactfcmp/fsub/fadddouble body. It does not qualify for a typed-f64 clone(its body is not straight-line), so it keeps the ordinary f64 body.
14_closure—computetrades its_i64body forcompute__typed_f64+compute__genericbehindjs_typed_f64_arg_guard.Census.
compiler_output_regression.py census --gategreen on bothcompilers, and the per-workload per-representation tables are byte-identical
between arms. No floor lowered, no
--update.Corpus sweep. All 467
test-files/test_gap_*.tscompiled and run underboth compilers with program output compared. 459 ran in both arms; exactly two
differ. One is the new test. The other is
test_gap_console_methods, whosediff is entirely
console.timedurations (timer1: 0.025msvs0.024ms) —the parity normalizer rewrites those to
<timer>. The 9 that did not run(
test_gap_http*,test_gap_net*,test_gap_fetch_*,test_gap_regex_replace_dyn_regex_with_http) failed to compile identicallyin both arms under the ad-hoc invocation; no error is asymmetric.
Sabotage. Restoring the pass (
i64_spec.rs+i64_emit.rs+is_integer_specializable+ thecodegen/mod.rswiring) reds 3 of the 4 newunit tests. The 4th (
fractional_literal_body_stays_on_the_double_path) staysgreen by design — the old gate already rejected fractional literals inside a
body (#6221); its point is that the parameter hole was the one left open.
Cost, stated plainly
05_fibonacciregresses. Interleaved A/B on a Mac mini, 9 pairs, fixed armslower in 9/9: median 450 ms → 555 ms for
fib(40)(~20%).14_closureiswithin noise (59 → 63 ms median, both series drifting with host load).
Two caveats, both real. Neither host was quiet, so these are indicative rather
than controlled. And both arms were built
--profile perry-dev(opt-level=1,no LTO) — the arm-to-arm ratio is what the interleaving buys, but a
releasebuild could narrow or widen it, and I did not measure that. For scale: Node
26.5.1 runs the same
fib(40)in ~1500 ms on that host, so the fixed arm isstill ~2.7x faster than Node (the base arm was ~3.3x).
That is a real cost, and it is the right trade: the pass was computing
different numbers than the language specifies, on ordinary reachable code, with
no proof available to narrow it. A sound recursive-numeric specialization —
guarded entry plus a deopt edge to the f64 body — is worth building; it is a
different piece of work from this one; filed as #7244 with the measurement,
the design it needs (guarded entry, a preserved f64 body to deopt into, and a
checked interior), and an acceptance bar.
Gates
cargo test -p perry-codegen --lib— 544 pass (540 + 4 new).i64_spec_ternary_recursion2/2.native_proof_regressions235 pass / 16 fail — the same 16 at
origin/mainwith an unmodifiedtree, so pre-existing and untouched by this diff (15
invalidation::*buffer-bounds tests plus
typed_f64_receiver_method_clone_raw_loads_after_composed_guards).cargo fmt --all; file-size gate 16 offenders on both arms, symmetricdifference empty.
specialization pass, its admission predicate, and the rejection-reason
variant that only it produced. The GC stress matrix does not apply.