fix(codegen): i32 fast path must prove a copied local is i32-RANGED, not merely integer (#6339)#6365
Merged
Merged
Conversation
…not merely integer (#6339) The strictly-i32-bounded oracle answered its LocalGet/Update arms from integer_locals, which means integer-VALUED and deliberately admits overflowing Add/Sub/Mul. A local holding an overflowed arithmetic result therefore still vouched for its copies, so 't = big2' (big2 = 2e9 + 2e9) took an i32 shadow and truncated to -294967296. Turn the set into the greatest fixpoint of 'every write is i32-bounded': seed the oracle optimistically with integer_locals, record which strict verdicts merely leaned on the oracle, and propagate every disqualification backwards along those copy edges with a worklist (one walk, linear). Intentionally-i32 code is untouched: | 0, >>> 0, bitwise ops, Math.imul, clamp calls and byte loads all prove themselves without consulting the oracle, so image_convolution's FNV-1a accumulator keeps its i32 slot.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesi32 boundedness provenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant i32LocalCollector
participant StrictnessOracle
participant RegressionTest
Compiler->>i32LocalCollector: collect strictly bounded locals
i32LocalCollector->>StrictnessOracle: evaluate writes with oracle dependencies
StrictnessOracle-->>i32LocalCollector: strict results and copy edges
i32LocalCollector->>i32LocalCollector: propagate disqualifications to a fixpoint
i32LocalCollector-->>Compiler: remaining bounded locals
RegressionTest->>Compiler: execute arithmetic and indexing cases
Compiler-->>RegressionTest: produce expected values
Possibly related issues
Possibly related PRs
🚥 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 |
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
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.
Closes #6339 — the fourth and last known face of #6072 (
i32 fast-path locals silently wrap at 2^31), after #6258 (barex++accumulator), #6318 (dynamic-bound loop counter) and #6319 (out-of-range integer literal).Root cause
is_strictly_i32_bounded_expr'sLocalGetarm answered frominteger_locals:That conflates two different properties.
integer_localsmeans integer-valued (no fraction, no NaN) and, by its own doc comment, deliberately admits overflowing arithmetic:big2itself stayed correct — anAddwrite is not strictly-i32-bounded, so it never got a shadow — yet the oracle still vouched for it.t = big2was therefore judged a strictly-i32-bounded write,tgot an i32 shadow at itsLetsite, and the store truncated. Reads prefer the shadow (#48), so the wrapped value is whatconsole.logsaw.The fix: greatest fixpoint
The set is defined by mutual recursion — a copy
t = big2is strict iffbig2is i32-ranged, which is this very set. So seed the oracle optimistically withinteger_localsand take the greatest fixpoint of "every write is i32-bounded".is_strictly_i32_bounded_expris monotone in its oracle, so starting at the top and only ever removing is exact and terminating.It is computed in one walk, not by re-walking per removal (that would be super-linear on big functions — cf. #6219/#6253). The judgment is a shallow match and only two of its arms consult the oracle, so each strict write records the exact local it leaned on (
copy_deps), and a worklist propagates every disqualification backwards along those edges. Same provenance/worklist shapecollect_integer_localsalready uses one module over.Judgment and provenance come from one match arm-set, so they cannot drift apart (the #6221 lesson): a future oracle-consulting arm is automatically both judged and tracked.
Why the FNV / imul32 recovery is untouched
strictly_i32_bounded_localsis what recovers image_convolution's FNV-1ahaccumulator, and shrinking it is exactly what thecompiler-output-regressiongate would punish. It doesn't shrink there, and the reason is structural:Only the
LocalGetandUpdatearms consult the oracle. Every other arm is a self-contained proof — an in-range literal, an explicit| 0/>>> 0coercion, a bitwise op,Math.imul, a clamp call, a byte load. Intentionally-i32 code proves itself and never asks the oracle anything, so no amount of oracle-shrinking can cost it its slot:A value that merely happens to overflow (
big1 + big1,sum += …,x++) has no such proof and reaches its copies only through the oracle — which is precisely the edge this PR cuts.That is the intended-vs-accidental separation, and it is load-bearing rather than incidental: it is the same structure #6340 relied on to leave
0x9E3779B9 >>> 0and0x811c9dc5 | 0alone while narrowing the bare-literal arm.compiler-output-regression— run locally, every CI stepRan with the exact CI invocations (
.github/workflows/test.yml), and A/B'd every structural report against a pristine baseline build:tests.test_compiler_output_regressiontests.test_native_abi_evidence_reportnative-region-proof(incl.image_convolution)failed_workloads: []native-abi-prooffailed_workloads: []tests.test_typed_feedback_runtime_evidencevectorized_buffer_transformhir_fact_rewritefma_contractBaseline-vs-this-PR, all 11
native-region-proofworkloads: 393 checks, identical statuses, and byte-identical named-region counters. image_convolution in particular:blurfnv_hashinput_generationfnv_imul_stays_i32,assembly_contains_imul_constantandnamed_region_fnv_i32_hash_shapeall still pass — the FNV chain still lowers toxor i32/mul i32 …, 16777619and the blur region still takes zero ToInt32 conversions.(
fma_contract's CI invocation passes--clang-arg=-march=haswell, which an arm64 host cannot accept; it errors identically on the baseline binary. Re-run without the x86-only flag it passes 22/22.)Verification
test-files/test_gap_6339_i32_arith_chain_copy.ts— byte-identical tonode --experimental-strip-types; fails on baseline with-294967296/705032704/1410065408/294967296/-2147483647.Covers: copy of
Add/Sub/Mulresults past 2^31, asum += …accumulator, a transitive copy chain (c2 = c1; c3 = c2; c4 = c3), copy-of-x++, the magnitude matrix (2^31-1stays i32;2^31,2^31+1,2^32,-2^31,-2^31-1,MAX_SAFE_INTEGERall exact), and the intentional-i32 shapes that must keep wrapping — FNV-1a,Math.imul, a hand-rolledimul32, a bit-mixing chain, an explicit(big1 + big1) | 0, a>>> 0xorshift recurrence, the array-index fast path (#6299/#6312) and image_convolution's clamp/blur index shape.test_gap_2308_unrolled_loop_varwas a shared-object-cache race between the two parallel compilers,test_gap_console_methodsisconsole.timejitter.)test_gap_6072_i32_update_overflow,test_gap_6072_dynamic_bound_counterand fix(codegen): an integer literal past 2^31 must not seed the i32 fast path (#6319) #6340'stest_gap_6319_i32_literal_magnitude: unchanged, byte-identical to node.cargo test -p perry-codegen,cargo fmt --all -- --check,scripts/check_file_size.sh: clean.Follow-up found while verifying
#6359 — a fifth, unrelated face: a
>>> 0uint32 assigned into a local whose other write is a signed literal takes the signed i32 slot and reads back negative (let d = 0; d = 4000000000 >>> 0→-294967296). It reproduces identically onmain, on #6340 and on this branch, so it is not part of this family and is not touched here. It is a signedness bug, not a magnitude bug.Summary by CodeRabbit
Bug Fixes
Tests