From 3082e5a842182e467ec27e69d56a7e6e1c3028ad Mon Sep 17 00:00:00 2001 From: Davide Angelocola Date: Sat, 17 Oct 2026 13:04:24 +0200 Subject: [PATCH] fix: Vector API sums match Rust (float order, I64 prefix overflow) Float sums add sequentially: lane-wise reduceLanes(ADD) has no defined order. The I64 sum vectorizes only when every element is in [-2^k, 2^k) with n * 2^k <= 2^63, so no prefix can overflow; otherwise it runs the checked per-element loop, dropping [MAX, 1, -1] as Rust does. The range test uses shift/xor/or because NEON has no 64-bit min/max (the min/max variant ran 2x slower than scalar; this one is within 10%). Closes #535 Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 1 + CLAUDE.md | 11 +- .../core/simd/VectorApiSimdOperations.java | 102 +++++++----------- .../vortex/core/simd/SimdOperationsTest.java | 90 ++++++++++------ .../simd/VectorApiSimdOperationsTest.java | 17 +-- docs/compatibility.md | 16 +-- 6 files changed, 115 insertions(+), 122 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27131f02f..ed8e0d85f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Cascading writes allocate about 13% less and run faster, from cheaper ALP-RD encoding, dictionary string statistics and float statistics. ([#543](https://github.com/dfa1/vortex-java/pull/543)) +- The opt-in Vector API kernels now sum floats and check 64-bit overflow exactly as Rust does. ([#535](https://github.com/dfa1/vortex-java/issues/535)) ## [0.17.1] — 2026-10-11 diff --git a/CLAUDE.md b/CLAUDE.md index a65d2c32b..19e7d155e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -41,8 +41,8 @@ core — everything lives under `io.github.dfa1.vortex.core.*`: core.compute FastLanes, PrimitiveArrays, Utf8Order core.simd SimdOperations (kernels shared by reader+writer, all eleven ptypes), AutoVectorizedSimdOperations (C2-shaped loops, the Rust-parity default), - VectorApiSimdOperations (opt-in via --add-modules jdk.incubator.vector; a few documented - differences from Rust, see docs/compatibility.md), SimdOperationsSupport#preferred() (picks one) + VectorApiSimdOperations (opt-in via --add-modules jdk.incubator.vector; Rust-parity results, + see docs/compatibility.md), SimdOperationsSupport#preferred() (picks one) core.fbs / core.proto — generated wire codecs + their runtimes reader — VortexReader, VortexHttpReader, VortexHandle, ReadRegistry, Chunk, ArrayStats, Zone, ScanOptions, RowFilter; file internals (Footer, Trailer, PostscriptParser, …) @@ -275,10 +275,9 @@ When stuck on encode/decode behavior, consult **in this order**: starting point and `withXxx` methods return copies. Never a record and never a public constructor — they hold callbacks or executors with no meaningful `equals`, and a constructor freezes the field list as API. -- **The Vector API implementation may differ from Rust in a few reductions** (decision recorded in - `docs/compatibility.md` and `VectorApiSimdOperations`): lane-wise float sums and per-lane `I64`/`U64` - overflow detection, instead of forcing scalar loops to keep Rust's exact order. It is opt-in - (`--add-modules jdk.incubator.vector`); the default `AutoVectorizedSimdOperations` stays Rust-parity. +- **The Vector API implementation is Rust-parity too** (#535): float sums are sequential and the `I64` sum + vectorizes only when no prefix can overflow, else runs the checked sequential loop. It is opt-in + (`--add-modules jdk.incubator.vector`) because the module is still incubating, not because it differs. - **Small public APIs.** Don't expose internals — when in doubt, leave it out or make it private. - **POM deps** grouped with comments: `` then ``, each with project-internal (`io.github.dfa1.vortex:*`) deps first, then external. Omit empty sections. diff --git a/core/src/main/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperations.java b/core/src/main/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperations.java index 1cd73ff35..188c0f323 100644 --- a/core/src/main/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperations.java +++ b/core/src/main/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperations.java @@ -22,29 +22,15 @@ /// The [SimdOperations] written with the incubating Vector API: explicit vector loops over the /// preferred species of the CPU, each with a scalar tail for the elements that do not fill a vector. /// The Vector API does the work, including the reductions; no kernel is forced back to a scalar loop -/// to protect an ordering, with the two exceptions below. +/// to protect an ordering, except where Rust's result depends on one: float sums are sequential and +/// `I64` sums fall back to a checked sequential loop when a prefix could overflow. /// /// Only [SimdOperationsSupport] instantiates it, and only when `jdk.incubator.vector` is on the /// module graph (`--add-modules jdk.incubator.vector`); without it this class fails to link and the /// [AutoVectorizedSimdOperations] stays in effect. A differential test pins every kernel to that /// implementation, which follows the Rust reference. /// -/// ## Where it differs from Rust (a deliberate decision) -/// -/// - **Floating-point sums** accumulate lane-wise and reduce once, so they add in a different order -/// from Rust's sequential `f64` sum. The result can differ in its last bits, and the JDK documents -/// `reduceLanes(ADD)` on floats as using "an arbitrary order of operations, which may even vary over -/// time", so it may also differ between runs and between CPUs with different vector widths. Integer -/// results are exact. -/// - **`I64`/`U64` sums** detect overflow on each lane's running sum, not on the sequential prefix Rust -/// checks (a checked add per element). Both can report an overflow the other does not, on arrays whose -/// partial sums approach the limits: for example `[MAX, 1, -1]` overflows Rust's prefix but not the -/// total. -/// - **`F16`** sums and min/max are scalar loops: the Vector API has no half-float lanes. -/// -/// Outputs derived from these (the zone-map sum statistic) therefore depend on whether the module is -/// enabled; the auto-vectorized implementation, used without the flag, is the Rust-parity one. See -/// `docs/compatibility.md`. +/// `F16` sums and min/max are scalar loops: the Vector API has no half-float lanes. /// /// The shape follows Hardwood's `VectorOperations` (Apache-2.0, hardwood-hq/hardwood): the /// preferred species, a main loop over whole vectors, and a scalar tail. @@ -998,35 +984,39 @@ private static long laneSum(IntVector acc) { return total; } - // Overflow is detected on each lane's running sum (the sign bit of (acc ^ sum) & (v ^ sum) is set - // when the addition overflowed), not on the sequential prefix Rust checks, so the two can disagree - // in both directions on arrays whose partial sums approach the limits. The lanes and the scalar - // tail are then combined with exact checked adds. + // Rust drops the sum when any sequential prefix overflows, which a lane-wise check cannot see ([MAX, 1, -1]). + // When every element lies in [-2^k, 2^k) with n * 2^k <= 2^63 no prefix can overflow, so the lanes sum + // exactly; otherwise the checked sequential loop decides. (v ^ v >> 63) < 2^k tests the range with + // shift/xor/or only, as NEON has no 64-bit min/max. Data far from the limits (counts, ids, small amounts) takes the vector path. private static OptionalLong sumSigned(long[] a) { + int log2n = a.length <= 1 ? 0 : Long.SIZE - Long.numberOfLeadingZeros(a.length - 1L); + int k = 63 - log2n; LongVector acc = LongVector.zero(LONGS); - LongVector overflow = LongVector.zero(LONGS); + LongVector magnitude = LongVector.zero(LONGS); int i = 0; for (; i + LONGS.length() <= a.length; i += LONGS.length()) { LongVector v = LongVector.fromArray(LONGS, a, i); - LongVector next = acc.add(v); - overflow = overflow.or(acc.lanewise(VectorOperators.XOR, next).and(v.lanewise(VectorOperators.XOR, next))); - acc = next; + acc = acc.add(v); + magnitude = magnitude.or(v.lanewise(VectorOperators.XOR, v.lanewise(VectorOperators.ASHR, 63))); } - if (overflow.compare(VectorOperators.LT, 0L).anyTrue()) { - return OptionalLong.empty(); + long total = acc.reduceLanes(VectorOperators.ADD); + long bits = magnitude.reduceLanes(VectorOperators.OR); + for (; i < a.length; i++) { + bits |= a[i] ^ (a[i] >> 63); + total += a[i]; } - long total = 0; - try { - for (long lane : acc.toArray()) { - total = Math.addExact(total, lane); - } - for (; i < a.length; i++) { - total = Math.addExact(total, a[i]); + if (bits >>> k == 0) { + return OptionalLong.of(total); + } + long s = 0; + for (long v : a) { + try { + s = Math.addExact(s, v); + } catch (ArithmeticException _) { + return OptionalLong.empty(); } - } catch (ArithmeticException _) { - return OptionalLong.empty(); } - return OptionalLong.of(total); + return OptionalLong.of(s); } private static OptionalLong sumUnsigned(long[] a) { @@ -1061,10 +1051,8 @@ private static OptionalLong sumUnsigned(long[] a) { return OptionalLong.of(total); } - // Lane-wise accumulation, reduced once at the end. The order of additions therefore differs from - // Rust's sequential f64 sum (and the JDK says reduceLanes(ADD) on floats may use "an arbitrary order - // of operations, which may even vary over time"), so the last bits of the result can differ from - // Rust's and from the auto-vectorized implementation's. F16 has no vector lanes: a plain loop. + // Sequential, as Rust: float addition is not associative and reduceLanes(ADD) has no defined order, so + // lanes would change the last bits. @Override public double sumFloating(Object values, PType ptype) { return switch (ptype) { @@ -1085,36 +1073,20 @@ public double sumFloating(Object values, PType ptype) { } private static double sum(float[] a) { - int parts = FLOATS.length() / DOUBLES.length(); - DoubleVector acc = DoubleVector.zero(DOUBLES); - int i = 0; - for (; i + FLOATS.length() <= a.length; i += FLOATS.length()) { - FloatVector v = FloatVector.fromArray(FLOATS, a, i); - for (int part = 0; part < parts; part++) { - DoubleVector d = (DoubleVector) v.convertShape(VectorOperators.F2D, DOUBLES, part); - acc = acc.add(d, d.eq(d)); - } - } - double total = acc.reduceLanes(VectorOperators.ADD); - for (; i < a.length; i++) { - if (!Float.isNaN(a[i])) { - total += a[i]; + double total = 0; + for (float v : a) { + if (!Float.isNaN(v)) { + total += v; } } return total; } private static double sum(double[] a) { - DoubleVector acc = DoubleVector.zero(DOUBLES); - int i = 0; - for (; i + DOUBLES.length() <= a.length; i += DOUBLES.length()) { - DoubleVector v = DoubleVector.fromArray(DOUBLES, a, i); - acc = acc.add(v, v.eq(v)); - } - double total = acc.reduceLanes(VectorOperators.ADD); - for (; i < a.length; i++) { - if (!Double.isNaN(a[i])) { - total += a[i]; + double total = 0; + for (double v : a) { + if (!Double.isNaN(v)) { + total += v; } } return total; diff --git a/core/src/test/java/io/github/dfa1/vortex/core/simd/SimdOperationsTest.java b/core/src/test/java/io/github/dfa1/vortex/core/simd/SimdOperationsTest.java index 1e9f0f4f6..ce9cb19cd 100644 --- a/core/src/test/java/io/github/dfa1/vortex/core/simd/SimdOperationsTest.java +++ b/core/src/test/java/io/github/dfa1/vortex/core/simd/SimdOperationsTest.java @@ -6,7 +6,6 @@ import io.github.dfa1.vortex.core.io.VortexFormat; import io.github.dfa1.vortex.core.model.EncodingId; import io.github.dfa1.vortex.core.model.PType; -import org.assertj.core.data.Offset; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -161,6 +160,22 @@ void sum_i64_overflowOfAPartialSumIsOverflowEvenIfTheTotalFits() { assertThat(result).isEmpty(); } + @Test + void sum_i64_prefixOverflowPastTheVectorWidthIsStillOverflow() { + // Given the [MAX, 1, -1] case buried in an array long enough to take the vector loop, where no + // single lane overflows + long[] values = new long[64]; + values[0] = Long.MAX_VALUE; + values[1] = 1L; + values[2] = -1L; + + // When + OptionalLong result = sut.sum(values, PType.I64); + + // Then + assertThat(result).isEmpty(); + } + @Test void sum_i64_fitsAtTheBoundary() { // Given @@ -810,11 +825,49 @@ void maxUnsigned_matchesReference(PType ptype) { } } + @Test + void sum_i64_nearTheFastPathBound_matchesReference() { + // Given, per length, values hugging the +-2^k edge where the vector path hands over to the checked loop + // (k = 63 - ceil(log2 n)): an off-by-one in k, or floor instead of ceil, flips the answer at some length + Random random = new Random(535); + for (int length : LENGTHS) { + int log2n = length <= 1 ? 0 : 64 - Long.numberOfLeadingZeros(length - 1L); + long edge = 1L << (63 - log2n); + long[][] pool = {{edge - 1, -edge}, {edge, -edge - 1}, {Long.MAX_VALUE, Long.MIN_VALUE}}; + for (int round = 0; round < 20; round++) { + long[] values = new long[length]; + for (int i = 0; i < length; i++) { + values[i] = random.nextInt(4) == 0 ? random.nextLong() >> random.nextInt(64) + : pool[random.nextInt(2)][random.nextInt(2)]; + } + if (length > 0 && random.nextBoolean()) { + values[random.nextInt(length)] = pool[2][random.nextInt(2)]; + } + + // When / Then + assertThat(sut.sum(values, PType.I64)).as("length=%d round=%d", length, round) + .isEqualTo(reference.sum(values, PType.I64)); + } + } + } + + @Test + void sum_i64_exactEdgeOfTheBound() { + // Given n * 2^k == 2^63 exactly (8 * -2^60), and three values whose sum passes MAX though n=3 rounds to 4 + long[] fits = new long[8]; + java.util.Arrays.fill(fits, -(1L << 60)); + long[] overflows = {(1L << 62) - 1, (1L << 62) - 1, (1L << 62) - 1}; + + // When / Then + assertThat(sut.sum(fits, PType.I64)).hasValue(Long.MIN_VALUE); + assertThat(sut.sum(overflows, PType.I64)).isEqualTo(reference.sum(overflows, PType.I64)).isEmpty(); + } + @ParameterizedTest @EnumSource(value = PType.class, names = {"I8", "U8", "I16", "U16", "I32", "U32", "I64", "U64"}) void sum_matchesReference(PType ptype) { // Given random data at every length; the 64-bit types hold values below 2^40 so no partial sum - // overflows, which is the case where lane-wise and sequential overflow checks must agree + // overflows: the vector path of I64, and U64 without carries for (int length : LENGTHS) { Object values = arrayWithRuns(ptype, length, 1, new Random(length * 13L)); if (values instanceof long[] longs) { @@ -930,8 +983,7 @@ void narrowInto_matchesReference(PType ptype) { @ParameterizedTest @EnumSource(value = PType.class, names = {"F16", "F32", "F64"}) void sumFloating_agreesWithTheReferenceToRoundingError(PType ptype) { - // Given values small enough that the sum is well inside the double range. The Vector API sums - // lane-wise, so the last bits legitimately differ from the sequential reference. + // Given values small enough that the sum is well inside the double range for (int length : LENGTHS) { Object values = arrayWithRuns(ptype, length, 1, new Random(length * 19L)); if (values instanceof float[] f) { @@ -952,10 +1004,8 @@ void sumFloating_agreesWithTheReferenceToRoundingError(PType ptype) { // When double result = sut.sumFloating(values, ptype); - // Then within a relative 1e-9 of the sum of the magnitudes (the error scale of reordering) - double scale = Math.max(1.0, magnitude(values, ptype)); - assertThat(result).as("%s length=%d", ptype, length).isCloseTo(expected, - Offset.offset(scale * 1e-9)); + // Then bit-identical: Rust adds sequentially, so any reordering would show in the last bits + assertThat(result).as("%s length=%d", ptype, length).isEqualTo(expected); } } @@ -1156,30 +1206,6 @@ private static Object narrowCarrier(PType ptype, int length) { }; } - /// The sum of the absolute values: the scale of the rounding error a reordered float sum can have. - private static double magnitude(Object values, PType ptype) { - double total = 0; - switch (values) { - case float[] f -> { - for (float v : f) { - total += Math.abs(v); - } - } - case double[] d -> { - for (double v : d) { - total += Math.abs(v); - } - } - case short[] h -> { - for (short v : h) { - total += Math.abs(Float.float16ToFloat(v)); - } - } - default -> throw new IllegalArgumentException(ptype.toString()); - } - return total; - } - private static byte[] filled(byte[] array, byte value) { Arrays.fill(array, value); return array; diff --git a/core/src/test/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperationsTest.java b/core/src/test/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperationsTest.java index 137760a07..a57e28642 100644 --- a/core/src/test/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperationsTest.java +++ b/core/src/test/java/io/github/dfa1/vortex/core/simd/VectorApiSimdOperationsTest.java @@ -6,8 +6,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assumptions.assumeTrue; -/// The Vector API implementation: the shared [SimdOperationsTest] contract, plus the documented -/// divergence from Rust (docs/compatibility.md). +/// The Vector API implementation: the shared [SimdOperationsTest] contract, plus the Vector-API-specific +/// cases where a lane-wise shortcut would diverge from Rust. class VectorApiSimdOperationsTest extends SimdOperationsTest { private final SimdOperations reference = new AutoVectorizedSimdOperations(); @@ -20,16 +20,19 @@ protected SimdOperations createSut() { } @Test - void sum_signed64_oppositeHugeValuesInOneLane_overflowPerLane_unlikeTheSequentialReference() { + void sum_signed64_oppositeHugeValuesInOneLane_matchTheSequentialReference() { // Given alternating huge values of opposite sign. Every vector length is even, so every positive - // value lands in the same lane: that lane's running sum overflows although the total is zero + // value lands in the same lane: a lane-wise overflow check would drop the sum although no + // sequential prefix overflows (Rust: 0). Catches a regression to per-lane detection long[] values = new long[64]; for (int i = 0; i < values.length; i++) { values[i] = i % 2 == 0 ? Long.MAX_VALUE / 2 : -(Long.MAX_VALUE / 2); } - // When / Then the documented difference from Rust: lane-wise overflow detection (docs/compatibility.md) - assertThat(reference.sum(values, PType.I64)).hasValue(0L); - assertThat(sut.sum(values, PType.I64)).isEmpty(); + // When + var result = sut.sum(values, PType.I64); + + // Then + assertThat(result).isEqualTo(reference.sum(values, PType.I64)).hasValue(0L); } } diff --git a/docs/compatibility.md b/docs/compatibility.md index 78b5a2e6c..5bbdc5755 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -50,18 +50,10 @@ Bulk loops (min/max, sums, run counting, widening, bit-packing, ...) live behind Two implementations exist: the default, plain Java loops shaped for C2's auto-vectorizer, and one written with the incubating Vector API, selected only when the JVM is launched with `--add-modules jdk.incubator.vector` (omitting the flag is the opt-out; the JVM prints -`WARNING: Using incubator modules` once). The default follows Rust. **The Vector API implementation is a -deliberate, documented departure in the cases below**, because forcing a scalar loop to keep Rust's exact -order would defeat the point of using the Vector API. - -| Kernel | Rust | Vector API implementation | Effect | -|--------|------|---------------------------|--------| -| Floating-point sum (zone-map `Sum`) | Sequential `f64` accumulation | Lane-wise accumulation, one reduction at the end. The JDK documents `reduceLanes(ADD)` on floats as using "an arbitrary order of operations, which may even vary over time" | The sum can differ in its last bits from Rust's and from the default implementation, and between CPUs of different vector width. Integer sums are exact. | -| `I64` / `U64` sum | A checked add per element: any overflowing prefix drops the sum | Overflow detected on each lane's running sum, then lanes and tail combined with checked adds | Can report an overflow Rust does not, or the reverse, when partial sums approach the limits (`[MAX, 1, -1]` overflows Rust's prefix but not the total). Narrower integer sums are exact in both. | -| `F16` sum and min/max | Scalar | Scalar | The Vector API has no half-float lanes. No difference. | - -Statistics derived from these (the zone-map sum) therefore depend on whether the module is enabled. The -other kernels give identical results in both implementations, pinned by a differential test. +`WARNING: Using incubator modules` once). Both give Rust's results, pinned by a differential test: +floating-point sums are sequential (lane-wise accumulation would change the last bits), and the `I64` sum +sums in vector lanes only when no prefix can overflow (every element in `[-2^k, 2^k)` with `n * 2^k <= 2^63`), otherwise it runs +Rust's checked add per element, so `[MAX, 1, -1]` is dropped in both. `F16` sum and min/max are scalar in both. Separate from the Vector API, found while comparing against the Rust source: