Repository navigation
fix(vml, simd): sign of zero, exp range/±inf, round ties-to-even; exp bit-identical on every backend incl. wasm - #368
Conversation
…0, exp ±inf, exp range) Every existing vsneg/vsceil/vstrunc/vsexp test uses fewer than 16 elements, so it only ever exercises the scalar tail. These run 0..=40 lengths so each value lands in the SIMD body too. All five fail on master. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg
… reduction
- vsneg, vsceil, vstrunc: the 16-lane bodies computed 0.0 - v, which
turns -0.0 into +0.0 (vsneg(+0), vsceil(-0.5), vsceil(-0.0),
vstrunc(-0.5), vstrunc(-0.0)). They now use Neg, a sign-bit flip on
every backend, and match their own scalar tails bit for bit.
- simd_exp_f32 (vsexp, and every caller of the polynomial):
* +inf returned 1.7e38; anything above ln(f32::MAX) now returns +inf.
* pow2n_from_int clamps n to 127, so every input above ~88.38 came
back halved. Those lanes now scale by 2^(n-1) and then 2.
* The lower clamp at -87.336 made -inf and every smaller input return
~1.18e-38. The clamp is now -110 and lanes with n < -126 scale by
2^(n+64) then 2^-64 (the first product is exact), so -inf gives
+0.0 and the subnormal range is correct.
* Range reduction uses Cody-Waite ln2 hi/lo with mul_add: x - n*ln2 in
one f32 product had an error larger than the polynomial's. The
polynomial is Horner with mul_add.
- vstanh: -two_x instead of 0.0 - two_x (no observable change; exp of
either zero is 1).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg
…exp = +inf - The overflow select never fired: clamping at OVERFLOW_X already makes poly*2^127*2 overflow to +inf (disable run stayed green), so it is gone. - simd::tests pinned the old saturating contract (exp(+inf) finite, exp(200) finite, exp(-inf) ~1.4e-38). Re-pinned to IEEE: +inf, +inf, +0.0. - The two GELU kernels (hpc::models::layers::gelu, gpt2 gelu_simd) computed tanh as (e-1)/(e+1), which is inf/inf = NaN once exp overflows (x >= ~10). They now use the equal form 1 - 2/(e+1). New tests fail with NaN on the quotient form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…cal on every backend mul_add is fused on AVX-512/AVX2+FMA/NEON/scalar but unfused on wasm (and implementation-defined with relaxed-simd), and round() breaks ties to even on AVX-512/NEON/wasm but away from zero on AVX2/scalar/nightly. Either made exp differ by an ulp between backends. The Cody-Waite split already makes the fused form unnecessary for accuracy, so the reduction and Horner use plain * and +, and n = floor(x/ln2 + 0.5). Same rule as gemm_f64_tiled. New test: every lane equals the same op sequence in scalar f32, bit for bit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg
…checks exp and round F32x16/F64x8::round used f32::round (ties away from zero) on the AVX2, scalar and nightly backends but ties-to-even on AVX-512, NEON and wasm, so vsround's SIMD body disagreed with its own round_ties_even tail on AVX2 (vsround(0.5) = 1.0). All backends now use ties-to-even. wasm-simd-parity selfcheck: simd_exp_f32 equals the unfused scalar sequence bit for bit under node (code 70), and round breaks ties to even (code 71). With the native unit test on the same sequence, exp is bit-identical between wasm and native. The nightly change is untested here (no nightly toolchain); CI checks it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg
Found while building the ONNX facade in lance-graph (#1754):
vml::vsnegcould not be an alias for ONNXNeg, andvml::vsexpwas wrong at +inf. Every existing test of these kernels used fewer than 16 elements, so none ever reached the 16-lane SIMD body.Defects (each reproduced by a failing test before its fix)
vsneg+0.0+0.0-0.0vsceil-0.5,-0.0+0.0-0.0vstrunc-0.5,-0.0+0.0-0.0vsexp/simd_exp_f32+inf,2001.7e38+infvsexpx > ~88.38ln(f32::MAX)vsexp-inf,x < -87.34~1.18e-38+0.0, with correct subnormals in betweenvsround(AVX2/scalar/nightly arms)0.5,2.5,-0.51.0,3.0,-1.0(body ≠ its ownround_ties_eventail)0.0,2.0,-0.0models::layers::gelu, gpt2gelu_simd)x ≳ 10once exp overflows to +infNaN(inf/inf)xChanges
0.0 - v. They now useNeg, which flips the sign bit on every backend.simd_exp_f32:[-110, ln(f32::MAX)]. Lanes withn = 128use2^(n-1)·2; lanes withn < -126use2^(n+64)·2^-64. Each first product is exact, so each result is rounded once.LN2_HI = 355/512, 9 significant bits, son·LN2_HIandx − n·LN2_HIare exact).gemm_f64_tiledinsimd_opsalready uses for its reference tier:*and+, nevermul_add. Vectormul_addis fused on AVX-512, AVX2+FMA, NEON, scalar and nightly, but unfused on wasm; withrelaxed-simdits fusion is implementation-defined.n = floor(x/ln2 + 0.5)instead ofround().roundbroke ties differently per backend (next item).F32x16/F64x8::roundbreaks ties to even on every backend. AVX2, scalar and nightly usedf32::round(ties away from zero); AVX-512roundscale, NEON and wasm already used ties-to-even, and the docs already said so.1 − 2/(e+1).simd::testsasserted the old saturating exp contract. They now assert IEEE+inf/+inf/+0.0.check_exp_and_round, which runs under node. Return code 70 means exp ≠ the unfused scalar sequence bit for bit; 71 meansrounddoes not break ties to even.Evidence
896d287(5 failing on master) and4519689(vsround: red on the AVX2 arm, green on AVX-512).simd_exp_f32_matches_the_unfused_scalar_sequence_bit_for_bitchecks every lane against the same op sequence in scalar f32. Disables:round()instead of floor, a fused lo step, and a fused Horner each turn it red.round()back in, it fails with rc=70. Together with the native test, this gives exp wasm == native bit for bit.scripts/masking-parity.sh wasmstill passes.n−1path halvesvsexp(88.38); dropping then+64path givesvsexp(-inf) = 1.45e-38. An explicit overflow mask stayed green under its disable, so it was removed as unreachable.cargo test --release --lib: native (AVX-512) 2586 passed;--config .cargo/config-v3.toml(AVX2) 2535 passed. Clippy-D warningsand fmt are clean.checkrow covers the per-laneround_ties_evenchange).Not changed (findings, for review)
Census of all 79 non-test
mul_addsites. Scalarf32::mul_addis fused on wasm too, so only vectormul_adddiverges.hpc::cam_pq::squared_l2(codebook argmin), and the gpt2/openchat logits (greedy argmax).reduce_sum, whose order differs on wasm (balanced tree).gemm_f64_tiled. That is a separate decision.simd_half's BF16/F16fmauses scalarf32::mul_addand RNE conversion, so it is already consistent.🤖 Generated with Claude Code
https://claude.ai/code/session_01G22yT6htkcdyXsihxxXdrg