fix(codegen): keep the high word of i128 constants in the split-module IR reader - #10566
proggeramlug wants to merge 2 commits into
Conversation
…e IR reader The in-process dialect reader, which builds every function of a split (multi-codegen-unit) module, materialized integer operands with `IntType::const_int(v as u64, v < 0)`. That API takes a single 64-bit word, so an `i128` operand kept only its low word. BigInt literals that fit in i128 lower to exactly such operands (`NativeRep::SmallBigInt`), so in a split module `1180591620717411303424n` (2^70) read back as `0n` and a 97-bit negative literal became a different 64-bit value, while single-unit builds (LLVM's own text parser) were correct. Widths above 64 bits now build both two's-complement words with `const_int_arbitrary_precision`, matching the assembler's semantics; the unsigned full-width spelling LLVM accepts is parsed too. Tests: a dialect unit test that builds every constant operand form codegen emits through the typed and the line paths and compares each against LLVM's own parse of the same text (only the i128 forms diverged); a unit test that lowers wide BigInt literals through the real emitter and checks the words passed to js_bigint_from_i128_parts; an integration test compiling the issue repro with PERRY_CODEGEN_UNITS=2 against Node's output; a gap test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change preserves high and low words for wide integer constants in the in-process dialect reader. It adds LLVM reader validation, emitter checks, split-module integration coverage, and BigInt gap cases through 256-bit values. ChangesWide integer constants
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The split-module wide-BigInt fix preserves supported 128-bit constants without an actionable remaining regression risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
|
Landed via merge train #10610 (v0.5.1594). All source commits preserve authorship; merged main matches the validated train exactly. |
Summary
A BigInt literal wider than 64 bits changed value when its module was compiled as more than one codegen unit:
2n ** 70n === 1180591620717411303424nwasfalse, the literal printed0, and a 97-bit negative literal became adifferent 64-bit number. Single-unit builds were correct, so nothing in the gap/parity corpus (all single-unit) could
see it, while large real modules split on their own — every split module containing a >64-bit BigInt literal silently
computed with the wrong constant (curve primes, group orders, field moduli).
Root cause
crates/perry-codegen/src/dialect/types.rs:299(pre-fix):The in-process dialect reader — which builds every function of a split module natively through the LLVM C API, and is
the default only for split modules (
native_emit::native_units_mode) — materialized every integer operand withIntType::const_int, which takes a single 64-bit word. For ani128operand that keeps only the low word(
v as u64), sign-extended when the literal was negative.Every BigInt literal that fits in
i128lowers to exactly such operands:expr/mod.rs'sSmallBigIntrep spells theliteral as an
i128constant andnative_value/materialize.rs::box_small_bigint_i128_to_js_valuesplits it withtrunc i128 C to i64/ashr i128 C, 64. With the low word only,1180591620717411303424(2^70) became0and-98765432109876543210987654321became-18444665141527514289. LLVM's own assembler (the single-unit text path, andthe external-clang path) reads the literal at arbitrary precision and then sign-/zero-extends it to the operand width,
which is why only split modules were wrong.
This is the same class as #8228/#8241: a form the closed-set reader gets wrong is invisible to every per-PR job,
because the reader only runs by default on modules big enough to split.
The fix
dialect/types.rsnow builds integer constants with the assembler's semantics: widths ≤ 64 keep the existingsingle-word path, and wider widths build both two's-complement words via
const_int_arbitrary_precision. Thenon-negative case parses as
u128, so the full-width unsigned spelling LLVM also accepts is handled; an integer typewider than
i128(whichbasic_typecannot name today) is refused loudly rather than silently truncated.Tests (each fails on the baseline,
7661bc05fe)crates/perry/tests/issue_10545_split_unit_wide_bigint_literals.rs— compiles the issue repro withPERRY_CODEGEN_UNITS=2and asserts Node 26.5.1's byte-exact output, after asserting bothmain_ts.unit{0,1}.native.llexist (so a build that stopped splitting, or stopped routing split units through thereader, cannot pass vacuously). Baseline:
left: "false false 0\n12345678901234567890 -18444665141527514289 false\n18446744073709551615 …"; with the fix itmatches Node.
dialect::tests::constant_operands_match_llvms_own_parse_on_typed_and_line_paths— a table of every constantoperand form perry-codegen emits in a function body (i1…i128 including the spellings past a narrow type's signed
range,
nanbox::double_literal's output for each class it distinguishes, NaN-boxed tag words and a signalling NaNin hex,
float,null/undef/poison/zeroinitializer, theptrtointconstant expression,<2 x i64>and<4 x i32>literals) is built through both reader paths (typedFnStream::itemand the line reader) and eachis compared against LLVM's own parse of the same text, not against expectations written in the test. On the
baseline only the
i128rows diverge (i128 1180591620717411303424→i128 0,i128 170141183460469231731687303715884105727→i128 18446744073709551615, …); everything else already matched,which is the audit the issue asked for.
dialect::tests::wide_bigint_literal_words_survive_native_construction— drives the real emitter(
compile_moduleover a HIR module with wide BigInt literals), asserts thei128operand is still emitted, thenre-builds the module through the reader and checks the words handed to
js_bigint_from_i128_parts.test-files/test_gap_10545_split_unit_wide_bigint_literals.ts— the repro plus the 64-bit-boundary controls,the i128 edges in every radix, literals past i128 (secp256k1
p/n), and wide literals in closures, containers,class fields, default parameters and a loop. The gap harness compiles single units, so this pins the semantics
rather than reproducing the bug; compiled by a baseline binary with
PERRY_CODEGEN_UNITS=2it diverges from Nodeon the first line and dies partway through with
RangeError: Division by zero(a modulus literal truncated to 0).Validation (Linux x64, Node 26.5.1, baseline
7661bc05fe)cargo test --release -p perry-codegen --testscargo test --release -p perry --test issue_10545_… --test issue_10152_cgu_str_bytesscripts/run_lint_gates.sh(full, incl. compile tier)benchmarks/ci_public_baseline_check.py), which fails identically on a clean7661bc05feworktree — pre-existingrun_gap_tests.sh, 820 tests)GAP_EXIT=0, snapshot OK, 6 known non-passing — the same six as the baseline run, no new failures; the new gap test passescargo fmt --all -- --checkPerformance
Compile-time A/B of the compiler binary built from this same clone with and without the one-file change (both builds
bit-identical on rebuild),
PERRY_CODEGEN_UNITS=2 --no-link,perf stat -e instructions, 3 runs, mean:@noble/curvessecp256k1 program (wide curve constants)Runtime, BigInt-literal-heavy loop (300k iterations over >64-bit literals and a 256-bit modulus),
perf stat, 3 runs,mean instructions:
Node wall for the same loop: 0.10–0.14 s; the Perry binary's task-clock is ~0.125 s.
Package check (informational)
@noble/curves2.2.0 +@noble/hashes2.2.0, secp256k1getPublicKey/sign/verifywith a fixed secret key,compiled with
PERRY_CODEGEN_UNITS=2: still blocked, but by an unrelated defect that reproduces at 1 unit too —TypeError: param "allowInfinityPoint" is invalid: expected own property. Minimal repro (a closure created in an arrowfunction sees the first call's parameter value on the second call):
@noble/curves1.2.0'sReferenceError: Cannot access 'wnaf' before initializationalso persists on this build(1 unit and 2 units). Neither is touched by this PR; both are separate issues.
Not verified
PERRY_LLVM_INPROCESS=0) needsPERRY_LLVM_OPTon this host; withPERRY_LLVM_OPT=/usr/bin/opt-22it compiles the repro correctly, which is what confirmed the reader (not codegen)as the culprit.
c"…"byte strings, aggregate initializers, globalinitializers) are unchanged: LLVM's own parser reads the skeleton on every path.
Fixes #10545
Summary by CodeRabbit
Bug Fixes
Tests