[MOD-17526] Make the SQ8 metadata describe the reconstruction - #1011
Merged
Conversation
dor-forer
marked this pull request as ready for review
August 10, 2026 14:42
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1011 +/- ##
=======================================
Coverage 97.18% 97.18%
=======================================
Files 141 141
Lines 8420 8432 +12
=======================================
+ Hits 8183 8195 +12
Misses 237 237 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lerman25
reviewed
Aug 13, 2026
lerman25
left a comment
lerman25
left a comment
Collaborator
There was a problem hiding this comment.
Nice - 1 blocking comment
lerman25
reviewed
Aug 13, 2026
| // FP32 arithmetic could leave the representable range for input that is entirely valid. | ||
| // [-FLT_MAX, +FLT_MAX] made max - min overflow to inf, then delta inf, inv_delta 0, and | ||
| // finally inf * 0 = NaN, whose conversion to an integer is undefined behaviour. Doubles | ||
| // cannot overflow for any pair of finite floats, so the whole class disappears rather than |
Collaborator
There was a problem hiding this comment.
Blocking: WithNorm can still make min_val/max_val non-finite before this double range calculation. Both find_min_max() and transformed_value() compute input[i] - mean[i] in FP32. For example, finite FP32 input [FLT_MAX, 0] with finite mean [-FLT_MAX, 0] centers to [+Inf, 0]; this then gives diff = Inf, delta = Inf, and inv_delta = 0, so to_byte(+Inf) evaluates Inf * 0 as NaN. std::clamp preserves NaN, and the following conversion to uint32_t is undefined behavior. This is reachable by the mean-centred SQ8 configuration introduced by #1007, so the finite-input safety claim is incomplete. Please perform/check centering in a representation that cannot overflow here (or reject non-finite derived values before quantization) and add a UBSan regression for this case.
Collaborator
Author
There was a problem hiding this comment.
Agreed, and fixed in 6c8d6e0. You are right that the double range does not help when the value is already lost upstream in FP32.
find_min_max now clamps its endpoints to the float range. That is also a storage constraint rather than only a guard: min_val is stored as FP32, so a non-finite endpoint could not be stored under any arithmetic. Only the endpoints are clamped, so the per-element loop stays FP32 and pays nothing.
With min_val finite and inv_delta finite and positive, NaN is unreachable in to_byte: a centered inf element gives inf * finite = inf, not 0 * inf, and lands on 255. I also moved the bound from std::clamp to std::fmin/std::fmax, since clamp propagates NaN, so the conversion is defined without relying on a proof that spans two functions.
I went with clamping rather than centering in double. The tradeoff: it puts overflowing elements at the ends instead of placing them proportionally. Happy to switch to double centering if you want proportionality, but it costs a conversion per element and the FP32 min slot still cannot hold the range.
UBSan regression added: QuantizationHandlesNonRepresentableCenteredRange, with your exact input.
Collaborator
Author
There was a problem hiding this comment.
Correcting the record on this thread, because my earlier reply points at a commit that is no longer here.
That reply said this was fixed in 6c8d6e0d, which clamped the find_min_max endpoints. This branch was since rewritten to a much smaller change, and 6c8d6e0d is not an ancestor of the current head. So the fix it describes is not in the code, and the thread should not be resolved on the strength of it.
The undefined behaviour you identified is closed, by a different mechanism. Tracing your exact input through the current head:
input [FLT_MAX, 0], mean [-FLT_MAX, 0] -> centered [+inf, 0]
find_min_max -> (0, inf)
diff = inf, delta = inf, inv_delta = 1/inf = 0
to_byte((inf - 0) * 0) = to_byte(inf * 0) = to_byte(NaN)
and to_byte, from #1015 which is now on main:
if (!(scaled > MetadataType{0})) {
return OUTPUT_TYPE{0}; // zero, negative, -inf, or NaN
}!(NaN > 0) is true, so NaN takes the first branch and returns 0. Defined, with no conversion of a non-finite value. This works because the safe constant sits in the false branch of the comparison, which is also why that code uses ternaries rather than std::clamp or std::fmin/std::fmax — as you noted, std::clamp propagates NaN, and fmin/fmax propagate it too in this composition, besides costing two libm calls per element.
So the endpoint clamping was one way to reach a defined conversion, and the merged conversion guard is another. The regression test with your input, QuantizationHandlesNonRepresentableCenteredRange, is present on this branch and passing.
What is not fixed, and I do not want to imply otherwise. The metadata in that case is still degenerate: delta is infinite, so the whole vector quantizes to one value. That is defined but useless, and it is outside this PR, which changes only what the sums describe. It is tracked in MOD-17838 along with the rest of the intermediate-overflow policy: max - min overflow, input - mean overflow, delta underflow and reciprocal overflow, the FP32 sum accumulations, and whether non-finite input should be rejected at the public boundary instead. Item 1 there is the root question, since rejecting non-finite input at ingestion would make all of these unreachable.
Also worth flagging on this PR as it now stands: it does not make L2 provably non-negative. Self-distance comes out at up to about 2.6 float epsilons below zero, measured at -9.77e-04 at dimension 512 against ||x_r||^2 ~ 3e+03. That is down from a systematic -3.08 at the same dimension, roughly four orders of magnitude better, but it is not a sign guarantee, and the withdrawn integer-metadata revision did provide one. If a caller takes sqrt of the result, that matters. A one-line std::max(0.0f, ...) in the L2 kernels would give the guarantee, and is only defensible now that the systematic error is gone: on main today it would have masked the real 0.4% error instead of float noise. Happy to add it here or as a follow-up, whichever you prefer.
This was referenced Aug 16, 2026
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
The uint8 kernels accumulate products or squared differences of bytes, so
the total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold
that, and all of them are on the plain int8/uint8 index paths that ship
today.
* IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the
scalar UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB
from dimension 33,026, while the comment claimed support to 2^16.
ret_t is now 64-bit for every element type. Keeping it signed means the
"1 - ip" in the wrappers stays signed arithmetic and cannot underflow,
and the int8 paths are unaffected. The L2 comment still carried the old
"at least 2 bytes wider" rationale and is corrected.
* UINT8_InnerProductImp returned float on NEON and SVE, which
accumulated exactly in integer lanes and then discarded it, exact only
to dimension 258 since 2^24 / 65025 = 258.
* AVX512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a
signed int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned
horizontal reduce back into a signed int, so the distance went negative
from the same dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were
already unsigned and are unchanged.
Note the accumulation itself was never the problem. The SIMD adds wrap
modulo 2^32 and are bit-exact, so the bit pattern was already correct; the
top bit was being read as a sign. An unsigned 32-bit reduce therefore costs
nothing over the original and is exact through dimension 66,051, twice the
old signed limit of 33,025.
Above 66,051 a 32-bit total genuinely does run out, so each kernel gains a
`bool Wide` template parameter selecting the epilogue: the narrow unsigned
32-bit reduce, or a widening one that zero-extends the lanes to 64 bits
first and cannot wrap at any dimension. The lanes are accumulated
identically either way. The choosers pick once per index, so no branch
enters the kernel.
Widening unconditionally would have been simpler and was measured rather
than assumed. On an Ice Lake-SP Xeon it costs 4 extra uops in the epilogue:
dim 32 +20%
dim 55-200 +8 to +11%
dim 256 +7%
dim 900-1024 +4 to +5%
15 repetitions, pinned core, two passes with the A/B order reversed; sign
and magnitude hold across both. The loop bodies are instruction-for-
instruction identical with the loop tops at the same 32-byte offset, so
this is the epilogue alone. Instruction count understates it, because the
widening reduce lengthens a dependency chain rather than adding throughput
work; that is also why the absolute delta grows at high dim, where fewer
calls overlap to hide the latency.
Selecting per dimension keeps that cost off every ordinary index. The price
is instantiating both variants: on the AVX512F_BW_VL_VNNI translation unit
at -O2, object size goes from 514,792 to 657,592 bytes, +27.7%, with 197
extra exported symbols. The narrow instantiation is unchanged at 40
instructions for residual 32, before and after, so the common case keeps
the full benefit.
To avoid a second case ladder, CHOOSE_IMPLEMENTATION now forwards trailing
arguments as further template arguments using __VA_OPT__, so the same
ladder serves kernels templated on <residual> and on <residual, Wide>
alike and every existing call site is untouched.
CHOOSE_UINT8_IMPLEMENTATION wraps the dimension test so each of the 15
uint8 call sites is a one-word change, and
CHOOSE_SVE_UINT8_IMPLEMENTATION does the same for the SVE ladder.
The SQ8-to-SQ8 inner product kernels reuse this helper, on main as much as
here, so they now pass Wide explicitly. They pass false: SQ8 is capped at
the same dimension independently, because its q_sum_squares metadata slot
is a uint32 holding 65025 * dim, so a widening reduce there would exceed
what the metadata itself can represent.
Split out of #1011 because none of this depends on the SQ8 metadata
contract that PR is changing, while all of it affects code reachable today.
#1011 does depend on this, through the helper above.
The regressions use all-255 bytes, the worst case, which makes the expected
value an exact integer: dimensions 33,026 and 40,000 for the narrow path,
which the old signed reduce got wrong, and 66,052 and 80,000 for the wide
path. The existing UINT8 suites stop at dimension 128, which is why all of
this went unseen; being SIMD-versus-scalar comparisons they would also have
agreed with each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with
delete and stored the trailing norms through unaligned float casts, so
measurements taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
QuantPreprocessor::quantize could execute undefined behaviour for input
the API accepts, in three ways that all end at the same cast.
The scale was derived entirely in FP32. For [-FLT_MAX, +FLT_MAX], every
component finite and accepted without validation, max - min overflowed to
inf, delta became inf, inv_delta 0, and the per-element product
inf * 0 = NaN, whose conversion to an integer is undefined. UBSan:
"-nan is outside the range of representable values of type 'unsigned
char'". The existing diff == 0 guard covers equal values, not overflow of
the subtraction. (MOD-17528)
Two more paths reach the same cast and were found in review of the
follow-up work:
* With WithNorm, centering is an FP32 subtraction, so finite input
against a finite mean can produce inf before any range is computed:
FLT_MAX against mean -FLT_MAX centers to 6.8e38. Widening the range
to double does not help, because the value is already lost upstream.
* delta is stored as FP32, and (float)(diff / 255) underflows to zero
for any diff below about 1.8e-43 while diff itself is nonzero, so
testing diff does not catch it. 1/delta was then inf, and the minimum
element, whose numerator is exactly zero, scaled to 0 * inf.
There is also no rejection path: nothing in VecSim validates finiteness,
and AddVector has no way to report "unquantizable", so the contract has to
be saturation rather than an error.
All three are closed by normalizing the endpoints once, immediately after
find_min_max, covering both the plain and WithNorm branches. Both
endpoints get a two-sided clamp behind an order check that catches NaN. A
one-sided clamp is not enough: for an all-+inf vector inf <= inf passes
the order check, so std::max(+inf, -FLT_MAX) would leave min at +inf and
store it. min is stored as FP32, so a non-finite endpoint could not be
represented under any arithmetic; this is the storage limit as much as a
guard.
That bounds diff at 6.8e38, so delta can be neither inf nor NaN and only
the underflow guard remains, as one comparison. inv_delta stays double,
not for precision, which needs only +/-0.5 in 255, but because an FP32
reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top
element to 255.
The per-element bound is written by hand rather than with std::clamp or
std::fmin/std::fmax. Each of those breaks something: std::clamp is
comparisons and propagates NaN into the cast, while fmin/fmax are
NaN-correct but compile to two out-of-line libm calls per element at this
translation unit's baseline. Measured at -O3: 8 instructions for this
form against 9 for std::clamp and 10 plus two calls for fmin/fmax. It
also subsumes the std::round that was there, since bounding first makes
+0.5 and truncation equivalent, and round() is likewise out-of-line here.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven domain matrix covering constant vectors positive negative and
zero, a subnormal but representable delta, a range that underflows and
collapses, the full FP32 range, all +inf, all -inf, mixed infinities, and
NaN first, middle and last. Expected bytes and metadata are asserted
rather than a range check, which is vacuous for uint8_t. The three NaN
cases pin that position matters: std::minmax_element compares with < and
every comparison against NaN is false, so a NaN at either end reaches an
endpoint and trips the order check while one in the middle is skipped and
the finite values set a real range. Expectations were derived by
simulating the pipeline, which corrected three of them.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
The uint8 kernels accumulate products or squared differences of bytes, so
the total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold
that, and all of them are on the plain int8/uint8 index paths that ship
today.
* IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the
scalar UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB
from dimension 33,026, while the comment claimed support to 2^16.
ret_t is now 64-bit for every element type. Keeping it signed means the
"1 - ip" in the wrappers stays signed arithmetic and cannot underflow,
and the int8 paths are unaffected. The L2 comment still carried the old
"at least 2 bytes wider" rationale and is corrected.
* UINT8_InnerProductImp returned float on NEON and SVE, which
accumulated exactly in integer lanes and then discarded it, exact only
to dimension 258 since 2^24 / 65025 = 258.
* AVX512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a
signed int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned
horizontal reduce back into a signed int, so the distance went negative
from the same dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were
already unsigned and are unchanged.
The accumulation itself was never the problem. The SIMD adds wrap modulo
2^32 and are bit-exact, so the bit pattern was already correct; the top bit
was being read as a sign. An unsigned 32-bit reduce therefore costs nothing
over the original and is exact through dimension 66,051, twice the old
signed limit of 33,025. Verified: the AVX512 object is byte-for-byte the
same size as before at 514,792, with the same 33 and 40 instructions for
residual 0 and 32.
Above 66,051 the choosers hand back the scalar kernel, which after the
ret_t change is exact to roughly dimension 2.8e14. That is one comparison
at index creation, reusing the "if (dim < 32) return ret_dist_func" idiom
the choosers already had, and it leaves every kernel untouched.
Two alternatives were explored and rejected, both recorded on the constant:
* Widening the horizontal reduce. Measured on an Ice Lake-SP Xeon it
costs 4 extra uops in the epilogue: +20% at dimension 32, +8-11% across
55-200, +4-5% at 900-1024, on byte-identical loop code. It also only
moves the limit, and to a different place per ISA, since NEON combines
four accumulators with vaddq_u32 in 32 bits before any widening reduce
sees them, capping it at 264,204 rather than the 1,056,816 AVX512 gets.
* Chunking the accumulation and flushing into a 64-bit total. Exact at
any dimension, and cheap when the chunk loop lives in the wrapper
rather than the kernel: +2 instructions on the fast path against +12 to
+21 when placed inside. Deferred rather than dismissed, since it is
only worth the restructuring if such dimensions become real.
Nothing comparable supports that range today, which is what settles it.
Lucene caps its scalar-quantized format at 1,024 dimensions and
Elasticsearch caps dense vectors at 4,096, both keeping a 32-bit
accumulator safe by contract. Faiss's QT_8bit_direct accumulates
full-range bytes into 32-bit lanes with no widening and carries the same
theoretical limit. Qdrant quantizes to 0..127, lowering the per-element cap
to 16,129, and its raw uint8 metric still sums into i32. The scalar
fallback here is already stricter than any of them.
Split out of #1011 because none of this depends on the SQ8 metadata
contract that PR is changing, while all of it affects code reachable today.
#1011 does depend on this, since its SQ8_SQ8 kernels call
UINT8_InnerProductImp. Note SQ8 is independently capped at the same
dimension, because q_sum_squares is a uint32 slot holding 65025 * dim, so
that PR needs its own fence regardless.
The regressions use all-255 bytes, the worst case, which makes the expected
value an exact integer, at dimensions 33,026 and 40,000 for the SIMD path
and 66,052 for the fallback. The fallback test asserts the returned
function pointer, not just the distance: on a host with no uint8 SIMD tier
the value comparison would pass either way, but the pointer identity would
not. The existing UINT8 suites stop at dimension 128, which is why all of
this went unseen; being SIMD-versus-scalar comparisons they would also have
agreed with each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with
delete and stored the trailing norms through unaligned float casts, so
measurements taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
QuantPreprocessor::quantize could execute undefined behaviour for input
the API accepts, in three ways that all end at the same cast.
The scale was derived entirely in FP32. For [-FLT_MAX, +FLT_MAX], every
component finite and accepted without validation, max - min overflowed to
inf, delta became inf, inv_delta 0, and the per-element product
inf * 0 = NaN, whose conversion to an integer is undefined. UBSan:
"-nan is outside the range of representable values of type 'unsigned
char'". The existing diff == 0 guard covers equal values, not overflow of
the subtraction. (MOD-17528)
Two more paths reach the same cast and were found in review of the
follow-up work:
* With WithNorm, centering is an FP32 subtraction, so finite input
against a finite mean can produce inf before any range is computed:
FLT_MAX against mean -FLT_MAX centers to 6.8e38. Widening the range
to double does not help, because the value is already lost upstream.
* delta is stored as FP32, and (float)(diff / 255) underflows to zero
for any diff below about 1.8e-43 while diff itself is nonzero, so
testing diff does not catch it. 1/delta was then inf, and the minimum
element, whose numerator is exactly zero, scaled to 0 * inf.
There is also no rejection path: nothing in VecSim validates finiteness,
and AddVector has no way to report "unquantizable", so the contract has to
be saturation rather than an error.
All three are closed by normalizing the endpoints once, immediately after
find_min_max, covering both the plain and WithNorm branches. Both
endpoints get a two-sided clamp behind an order check that catches NaN. A
one-sided clamp is not enough: for an all-+inf vector inf <= inf passes
the order check, so std::max(+inf, -FLT_MAX) would leave min at +inf and
store it. min is stored as FP32, so a non-finite endpoint could not be
represented under any arithmetic; this is the storage limit as much as a
guard.
That bounds diff at 6.8e38, so delta can be neither inf nor NaN and only
the underflow guard remains, as one comparison. inv_delta stays double,
not for precision, which needs only +/-0.5 in 255, but because an FP32
reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top
element to 255.
The per-element bound is written by hand rather than with std::clamp or
std::fmin/std::fmax. Each of those breaks something: std::clamp is
comparisons and propagates NaN into the cast, while fmin/fmax are
NaN-correct but compile to two out-of-line libm calls per element at this
translation unit's baseline. Measured at -O3: 8 instructions for this
form against 9 for std::clamp and 10 plus two calls for fmin/fmax. It
also subsumes the std::round that was there, since bounding first makes
+0.5 and truncation equivalent, and round() is likewise out-of-line here.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven domain matrix covering constant vectors positive negative and
zero, a subnormal but representable delta, a range that underflows and
collapses, the full FP32 range, all +inf, all -inf, mixed infinities, and
NaN first, middle and last. Expected bytes and metadata are asserted
rather than a range check, which is vacuous for uint8_t. The three NaN
cases pin that position matters: std::minmax_element compares with < and
every comparison against NaN is false, so a NaN at either end reaches an
endpoint and trips the order check while one in the middle is skipped and
the finite values set a real range. Expectations were derived by
simulating the pipeline, which corrected three of them.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
…nput QuantPreprocessor::quantize could execute undefined behaviour for input the API accepts. Three paths, all ending at the same conversion to a byte, and all reachable with entirely finite components. The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range overflowed: max - min became inf, delta inf, inv_delta 0, and the per-element product inf * 0 = NaN, whose conversion to an integer is undefined. UBSan: "-nan is outside the range of representable values of type 'unsigned char'". The existing diff == 0 guard covers equal values, not overflow of the subtraction. (MOD-17528) With WithNorm, centering is an FP32 subtraction, so finite input against a finite mean reaches inf before any range is computed: FLT_MAX against mean -FLT_MAX centers to 6.8e38. Widening the range does not help, because the value is already lost upstream. And delta is stored as FP32, so (float)(diff / 255) underflows to zero while diff itself is nonzero, which leaves 1/delta inf and scales the minimum element, numerator exactly zero, to 0 * inf. All three are closed by normalizing the endpoints once, after find_min_max, covering the plain and WithNorm branches together, after which a single delta comparison suffices. Both endpoints get a two-sided clamp: for an all-+inf vector inf <= inf passes the order check, so a one-sided std::max would leave min at +inf and store it. min is stored as FP32, so a non-finite endpoint could not be represented under any arithmetic. inv_delta stays double, not for precision, which needs only +/-0.5 in 255, but because an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta 2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element to 255. The per-element bound and rounding are written by hand. That replaces std::round, an out-of-line libm call at this translation unit's baseline that ran once per element: bounding first makes adding 0.5 and truncating equivalent for non-negative values. Measured at -O3 it is 8 instructions against 9 for std::clamp plus std::round's call. Deliberately NOT claimed: that the function is defined for non-finite components. It is not, and cannot be made so here. find_min_max uses std::minmax_element, whose precondition is that the comparison induce a strict weak ordering, and floating-point < is not one once a NaN is present: incomparability must be transitive, yet 1.0 is incomparable with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. The undefined behaviour is therefore inside that algorithm, before quantize() ever sees a range, so no normalization afterwards can define a portable result. A partial contract would also be misleading, since x_mean_ip, the quantized sums and the whole query metadata path are untouched and can still produce non-finite values. Non-finite components are treated as unsupported. The order check remains as a defensive fallback so that a NaN endpoint cannot be stored, which degrades a caller error into meaningless-but-finite metadata rather than poisoning every distance computed against that vector. Validating at the public ingestion boundary belongs in its own change; nothing in VecSim does it today. This matches how comparable systems handle it. Lucene validates vector components and throws on NaN or infinity, and Elasticsearch rejects NaN, infinity and magnitudes that overflow before delegating to Lucene. Faiss guards only an exactly zero range and assumes finite input otherwise, with the same float-to-integer concern at its final cast. Qdrant gets defined bytes from Rust's saturating cast, which C++ does not have, and can still store an unusable scale. Tests: the MOD-17528 reproduction, the WithNorm centering case, and a table-driven domain matrix over constant vectors positive negative and zero, a single element, a subnormal but representable delta, a range that underflows and collapses, the full FP32 range, and all-+inf, all--inf and mixed infinities. Infinities are pinned exactly, since < remains a strict weak ordering over finites and +/-inf; only NaN breaks it. Expected bytes and metadata are asserted rather than a range check, which is vacuous for uint8_t, and every expectation was derived by simulating the pipeline rather than predicted. NaN input gets one test that asserts only that the stored min and delta stay finite and delta positive, which is position-independent and portable, and which under UBSan also covers the conversion. Metadata meaning is unchanged: the sums are still FP32 over the input values. Making them exact integer sums over the quantized bytes is a storage-contract change that has to move together with every kernel that reads them, and stays in #1011. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
…nput QuantPreprocessor::quantize could execute undefined behaviour for input the API accepts. Three paths, all ending at the same conversion to a byte, and all reachable with entirely finite components. The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range overflowed: max - min became inf, delta inf, inv_delta 0, and the per-element product inf * 0 = NaN, whose conversion to an integer is undefined. UBSan: "-nan is outside the range of representable values of type 'unsigned char'". The existing diff == 0 guard covers equal values, not overflow of the subtraction. (MOD-17528) With WithNorm, centering is an FP32 subtraction, so finite input against a finite mean reaches inf before any range is computed: FLT_MAX against mean -FLT_MAX centers to 6.8e38. Widening the range does not help, because the value is already lost upstream. And delta is stored as FP32, so (float)(diff / 255) underflows to zero while diff itself is nonzero, which leaves 1/delta inf and scales the minimum element, numerator exactly zero, to 0 * inf. find_min_max now guarantees that both endpoints are finite and ordered, and everything else follows from that. The guarantee lives there rather than at the call site because that is where it can be broken: the WithNorm branch creates the inf itself, from two valid operands, and the plain branch passes through whatever the input holds. Both endpoints get a two-sided clamp, since for an all-+inf vector inf <= inf passes the order check and a one-sided std::max would leave min at +inf. Clamping to the float range is not merely defensive either: min is stored as an FP32 field, so a non-finite endpoint could not be represented under any arithmetic. With that established, quantize needs one delta comparison, and inv_delta stays double. Not for precision, which needs only +/-0.5 in 255, but because an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta 2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element to 255. The per-element bound and rounding are written by hand. That replaces std::round, an out-of-line libm call at this translation unit's baseline that ran once per element: bounding first makes adding 0.5 and truncating equivalent for non-negative values. Measured at -O3 it is 8 instructions against 9 for std::clamp plus std::round's call. Deliberately NOT claimed: that the function is defined for non-finite components. It is not, and cannot be made so here. find_min_max uses std::minmax_element, whose precondition is that the comparison induce a strict weak ordering, and floating-point < is not one once a NaN is present: incomparability must be transitive, yet 1.0 is incomparable with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. The undefined behaviour is therefore inside that algorithm, before any range exists, so no normalization afterwards can define a portable result. A partial contract would also be misleading, since x_mean_ip, the quantized sums and the whole query metadata path are untouched and can still produce non-finite values. Non-finite components are treated as unsupported. The order check remains as a defensive fallback so that a NaN endpoint cannot be stored, which degrades a caller error into meaningless-but-finite metadata rather than poisoning every distance computed against that vector. Validating at the public ingestion boundary belongs in its own change; nothing in VecSim does it today. This matches how comparable systems handle it. Lucene validates vector components and throws on NaN or infinity, and Elasticsearch rejects NaN, infinity and magnitudes that overflow before delegating to Lucene. Faiss guards only an exactly zero range and assumes finite input otherwise, with the same float-to-integer concern at its final cast. Qdrant gets defined bytes from Rust's saturating cast, which C++ does not have, and can still store an unusable scale. Tests: the MOD-17528 reproduction, the WithNorm centering case, and a table-driven domain matrix over constant vectors positive negative and zero, a single element, a subnormal but representable delta, a range that underflows and collapses, the full FP32 range, and all-+inf, all--inf and mixed infinities. Infinities are pinned exactly, since < remains a strict weak ordering over finites and +/-inf; only NaN breaks it. Expected bytes and metadata are asserted rather than a range check, which is vacuous for uint8_t, and every expectation was derived by simulating the pipeline rather than predicted. NaN input gets one test that asserts only that the stored min and delta stay finite and delta positive, which is position-independent and portable, and which under UBSan also covers the conversion. Metadata meaning is unchanged: the sums are still FP32 over the input values. Making them exact integer sums over the quantized bytes is a storage-contract change that has to move together with every kernel that reads them, and stays in #1011. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 16, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
… free
The uint8 kernels accumulate products or squared differences of bytes, so the
total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold that,
all of them on the plain int8/uint8 index paths that ship today.
* IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the scalar
UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB from
dimension 33,026, while the comment claimed support to 2^16. The
conditional was also dead: only int8_t and uint8_t instantiate these, both
1 byte, so it always selected int. ret_t is now 64-bit for every element
type, which also covers int8 at dimension 131,072. Keeping it signed means
the "1 - ip" in the wrappers stays signed arithmetic and cannot underflow.
The L2 comment still carried the old byte-counting rationale and is fixed.
* UINT8_InnerProductImp returned float on NEON and SVE, accumulating exactly
in integer lanes and then discarding it, exact only to dimension 258 since
2^24 / 65025 = 258.
* AVX-512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a signed
int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned horizontal
reduce back into a signed int, so the distance went negative from the same
dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were already unsigned.
The accumulation was never the problem. The SIMD adds wrap modulo 2^32 and are
bit-exact, so the bit pattern was already correct; the top bit was being read
as a sign. An unsigned 32-bit reduce therefore costs nothing over the original
and is exact through dimension 66,051, twice the old signed limit of 33,025.
Two bounds follow, because the horizontal total and the lanes run out at
different points:
* UINT8_NARROW_REDUCE_MAX_DIM = 66,051 bounds the 32-bit total, which is
floor(UINT32_MAX / 65,025). Past it the reduce is widened to 64 bits.
* UINT8_SIMD_MAX_DIM = 4 * 66,051 bounds the lanes, which widening the total
does not protect. NEON is the limiting ISA: it combines four accumulators
with vaddq_u32 in 32 bits before any widening reduce sees them, so its
capacity is four lanes' worth. AVX-512 accumulates into 16 lanes from one
accumulator and reaches roughly 1,056,816, and SVE depends on its vector
length, so NEON sets the shared bound for IP, Cosine and L2 alike. Above
it the choosers hand back the scalar kernel, exact by the ret_t change.
Only AVX-512 carries both reduce forms. On ARM widening is free, since
vaddlvq_u32 (UADDLV) and svaddv_u32 are single instructions already producing
64 bits, so those kernels always widen and need no variant.
The AVX-512 pair is two named wrappers, X and X_Wide, rather than a template
argument threaded through the chooser macros. implementation_chooser.h is
shared with every other element type, so keeping a uint8 concern out of it
avoids blast radius, and the narrow wrapper stays byte-for-byte what it was.
That matters, and was measured: putting a runtime branch in the epilogue
instead cost 0.4 to 0.6 ns per call, +20% at dimension 32 and +8% across
55..200 on an Ice Lake-SP Xeon, because the fatter function lost its inlining
in 31 of the Cosine wrappers and grew .text by 18.4%. Two names keep the
choice at index creation and both kernels branch-free. Selection lives inside
the per-ISA Choose_* functions, which already take dim, so no header changes
and no new exported names.
Verified against the narrow-only version: the narrow wrappers are unchanged at
33, 40 and 47 instructions for IP at residual 0, 32 and 33, and 37, 43 and 50
for Cosine, with zero calls in the 33..63 band and the out-of-line Imp count
unchanged at 7. The object file grows 514,792 to 656,120 for the extra 192
instantiations, which is the intended trade.
The SQ8_SQ8 kernels reuse this helper, on main as much as here. They now take
the result as uint64_t and pass Wide as false: SQ8 is capped at the same
66,051 independently, by its uint32 q_sum_squares metadata slot. Previously
the AVX-512 one assigned it to int, which wrapped past 33,025, and the three
ARM ones to float, which lost exactness past 258. Note the SQ8_SQ8 choosers
have no dimension guard, unlike the uint8 ones, so the fence belongs with SQ8
index creation in #1007; on main nothing constructs an SQ8 index.
Split out of #1011 because none of this depends on the SQ8 metadata contract
that PR is changing, while all of it affects code reachable today. #1011
depends on this, through the helper above.
Tests walk all four boundaries, at each bound and one past it, through the
dispatched function so selection is covered as well as arithmetic. All-255
against all-0 is the worst case and keeps every expectation an exact integer.
The top boundary is also asserted by pointer identity, since on a host without
a uint8 SIMD tier the value comparisons would pass either way, and the narrow
and widened dispatch results are asserted to differ so the selection is
exercised rather than assumed. The existing UINT8 suites stop at dimension
128, which is why all of this went unseen; being SIMD-versus-scalar
comparisons they would also have agreed with each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with delete
and stored the trailing norms through unaligned float casts, so measurements
taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
The uint8 kernels accumulate products or squared differences of bytes, so the
total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold that,
all of them on the plain int8/uint8 index paths that ship today.
* IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the scalar
UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB from
dimension 33,026, while the comment claimed support to 2^16. The
conditional was also dead: only int8_t and uint8_t instantiate these, both
1 byte, so it always selected int. ret_t is now 64-bit for every element
type, which also covers int8 at dimension 131,072. Kept signed so the
"1 - ip" in the wrappers stays signed arithmetic and cannot underflow. The
L2 comment still carried the old byte-counting rationale and is fixed.
* UINT8_InnerProductImp returned float on NEON and SVE, accumulating exactly
in integer lanes and then discarding it, exact only to dimension 258 since
2^24 / 65025 = 258.
* AVX-512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a signed
int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned horizontal
reduce back into a signed int, so the distance went negative from the same
dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were already unsigned.
The accumulation was never the problem. The SIMD adds wrap modulo 2^32 and are
bit-exact, so the bit pattern was already correct; the top bit was being read
as a sign. An unsigned 32-bit reduce therefore costs nothing over the original
and is exact through dimension 66,051 = floor(UINT32_MAX / 65,025), twice the
old signed limit of 33,025. Verified: the AVX-512 object is byte-for-byte the
same size as before at 514,792, with the same 33 and 40 instructions for
residual 0 and 32.
Above 66,051 the choosers hand back the scalar kernel, which after the ret_t
change is exact to roughly dimension 2.8e14. One comparison at index creation,
reusing the "if (dim < 32) return ret_dist_func" idiom the choosers already
had, and it leaves every kernel untouched.
Three alternatives were tried and rejected, each on evidence:
* Widening the horizontal reduce unconditionally. Measured on an Ice Lake-SP
Xeon it costs 4 extra uops in the epilogue: +20% at dimension 32, +8-11%
across 55-200, +4-5% at 900-1024, on byte-identical loop code.
* A runtime branch selecting the width per call. Measured at +0.4 to 0.6 ns
per call, +15% at dimension 32, and it cost 31 of the 65 Cosine wrappers
their inlining, growing .text by 18.4%.
* Compile-time selection between two named wrappers, extending SIMD to a
second bound of 4 * 66,051. This one is free on the narrow path, verified:
the narrow wrappers stayed byte-identical and the out-of-line count
unchanged. It was rejected for correctness, not cost. That bound assumes
products spread evenly across the four uint32 lanes after NEON's 32-bit
vaddq_u32 merge, and the even case already lands within 1,020 of
UINT32_MAX, while the masked residual load can add up to 16 products, or
1,040,400, into specific lanes. So lanes wrap before the widened reduce
sees them, and a correct bound would have to be derived per kernel from its
accumulator count and residual distribution. The narrow reduce needs none
of that: its bound is on the horizontal total, which does not depend on how
products land in lanes.
Recorded for whoever revisits this: on ARM the widening reduce is free
instruction-for-instruction. Cross-compiling with
clang++ --target=aarch64-linux-gnu -O2 emits addv/fmov w/ucvtf against
uaddlv/fmov x/ucvtf, three instructions either way. So the obstacle to a wider
band is the lane bound, not the reduce.
Nothing comparable supports that range regardless. Lucene caps its
scalar-quantized format at 1,024 dimensions and Elasticsearch caps dense
vectors at 4,096, both keeping a 32-bit accumulator safe by contract. Faiss's
QT_8bit_direct accumulates full-range bytes into 32-bit lanes with no widening
and carries the same theoretical limit. Qdrant quantizes to 0..127, lowering
the per-element cap to 16,129, and its raw uint8 metric still sums into i32.
The SQ8_SQ8 kernels reuse this helper, on main as much as here, so they now
take its result as uint32_t. Previously the AVX-512 one assigned it to int,
which wrapped past 33,025 once the helper stopped returning int, and the three
ARM ones to float, which lost exactness past 258. Note the SQ8_SQ8 choosers
have no dimension guard, unlike the uint8 ones, so the fence belongs with SQ8
index creation in #1007; on main nothing constructs an SQ8 index.
Split out of #1011 because none of this depends on the SQ8 metadata contract
that PR is changing, while all of it affects code reachable today. #1011
depends on this, through the helper above.
The regressions use all-255 bytes, the worst case, which makes the expected
value an exact integer, at dimensions 33,026 and 40,000 for the SIMD path and
66,052 for the fallback. The fallback test asserts the returned function
pointer, not just the distance: on a host with no uint8 SIMD tier the value
comparison would pass either way, but the pointer identity would not. The
existing UINT8 suites stop at dimension 128, which is why all of this went
unseen; being SIMD-versus-scalar comparisons they would also have agreed with
each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with delete
and stored the trailing norms through unaligned float casts, so measurements
taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
The uint8 SIMD kernels accumulate products or squared differences of bytes, so
the total reaches 255 * 255 * dim = 65025 * dim. Three paths discarded or
wrapped it:
* UINT8_InnerProductImp returned float on NEON and SVE, accumulating exactly
in integer lanes and then throwing that away, exact only to dimension 258
since 2^24 / 65025 = 258.
* AVX-512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a signed
int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned horizontal
reduce back into a signed int, so the distance went negative from the same
dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were already unsigned.
The accumulation was never wrong. The SIMD adds wrap modulo 2^32 and are
bit-exact, so the bit pattern was already correct; the top bit was being read
as a sign. Reading it unsigned therefore costs nothing, and doubles the exact
range from dimension 33,025 to 66,051 = floor(UINT32_MAX / 65,025). Verified:
the AVX-512 object file is byte-for-byte the same size as before at 514,792,
with the same 33 and 40 instructions for residual 0 and 32.
spaces::MAX_EXACT_UINT8_SIMD_DIM records that bound. It is documentation, not
a fence: above it these kernels still wrap, as they do on main, only twice as
far out. Two things are deliberately not done here.
Routing past the bound to the scalar kernel would need that kernel's
accumulator widened first. Its ret_t is std::conditional_t<sizeof(int_elem_t)
== 1, int, long long>, which for uint8 is int and therefore executes
signed-overflow UB from dimension 33,026 itself, so it is not a safe fallback
as it stands. That is a defect on the scalar path rather than in the SIMD
reduces this change is about, and it is filed separately.
Widening the SIMD reduce past the bound was implemented and measured, then
dropped. Unconditional widening cost +20% at dimension 32 on an Ice Lake-SP
Xeon, +8-11% across 55-200, on byte-identical loop code. A runtime branch cost
+0.4 to 0.6 ns per call and lost 31 of the 65 Cosine wrappers their inlining,
growing .text 18.4%. Compile-time selection between two named wrappers was
genuinely free on the narrow path, but its second bound of 4 * 66,051 assumed
products spread evenly across NEON's four uint32 lanes after the 32-bit
vaddq_u32 merge: the even case already lands within 1,020 of UINT32_MAX, while
the masked residual load can add 1,040,400 into one lane, so lanes wrap before
the widened reduce sees them. A correct bound would have to be derived per
kernel from its accumulator count and residual distribution. The unsigned
reduce needs none of that, because its bound is on the horizontal total, which
does not depend on how products land in lanes.
For whoever revisits it: on ARM the widening reduce is free
instruction-for-instruction. clang++ --target=aarch64-linux-gnu -O2 emits
addv/fmov w/ucvtf against uaddlv/fmov x/ucvtf, three instructions either way.
The obstacle is the lane bound, not the reduce.
Nothing comparable supports that range anyway. Lucene caps its
scalar-quantized format at 1,024 dimensions and Elasticsearch caps dense
vectors at 4,096. Faiss's QT_8bit_direct accumulates full-range bytes into
32-bit lanes with no widening and carries the same limit. Qdrant quantizes to
0..127 and its raw uint8 metric still sums into i32.
The SQ8_SQ8 kernels reuse the shared helper, on main as much as here, so they
now take its result as uint32_t. Previously the AVX-512 one assigned it to int
once the helper stopped returning int, which wrapped past 33,025, and the three
ARM ones to float, which lost exactness past 258.
Split out of #1011 because none of this depends on the SQ8 metadata contract
that PR is changing, while all of it affects code reachable today. #1011
depends on this, through the helper above.
The regression asserts the dispatched SIMD path at dimensions 33,026, 40,000
and 66,051, using all-255 against all-0 so the expected value is an exact
integer, and checks the distance is positive since going negative is the
symptom a user would have seen. It deliberately does not call the scalar
kernels, which remain undefined above 33,025. The existing UINT8 suites stop at
dimension 128, which is why this went unseen; being SIMD-versus-scalar
comparisons they would also have agreed with each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with delete
and stored the trailing norms through unaligned float casts, so measurements
taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
The uint8 kernels accumulate products or squared differences of bytes, so the
total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold that,
all of them on the plain int8/uint8 index paths that ship today.
* IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the scalar
UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB from
dimension 33,026, while the comment claimed support to 2^16. The
conditional was also dead: only int8_t and uint8_t instantiate these, both
1 byte, so it always selected int. ret_t is now 64-bit for every element
type, which also covers int8 at dimension 131,072. Kept signed so the
"1 - ip" in the wrappers stays signed arithmetic and cannot underflow. The
L2 comment still carried the old byte-counting rationale and is fixed.
* UINT8_InnerProductImp returned float on NEON and SVE, accumulating exactly
in integer lanes and then discarding it, exact only to dimension 258 since
2^24 / 65025 = 258.
* AVX-512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a signed
int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned horizontal
reduce back into a signed int, so the distance went negative from the same
dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were already unsigned.
The accumulation was never the problem. The SIMD adds wrap modulo 2^32 and are
bit-exact, so the bit pattern was already correct; the top bit was being read
as a sign. An unsigned 32-bit reduce therefore costs nothing over the original
and is exact through dimension 66,051 = floor(UINT32_MAX / 65,025), twice the
old signed limit of 33,025. Verified: the AVX-512 object is byte-for-byte the
same size as before at 514,792, with the same 33 and 40 instructions for
residual 0 and 32.
Above 66,051 the choosers hand back the scalar kernel, which after the ret_t
change is exact to roughly dimension 2.8e14. One comparison at index creation,
reusing the "if (dim < 32) return ret_dist_func" idiom the choosers already
had, and it leaves every kernel untouched.
Three alternatives were tried and rejected, each on evidence:
* Widening the horizontal reduce unconditionally. Measured on an Ice Lake-SP
Xeon it costs 4 extra uops in the epilogue: +20% at dimension 32, +8-11%
across 55-200, +4-5% at 900-1024, on byte-identical loop code.
* A runtime branch selecting the width per call. Measured at +0.4 to 0.6 ns
per call, +15% at dimension 32, and it cost 31 of the 65 Cosine wrappers
their inlining, growing .text by 18.4%.
* Compile-time selection between two named wrappers, extending SIMD to a
second bound of 4 * 66,051. This one is free on the narrow path, verified:
the narrow wrappers stayed byte-identical and the out-of-line count
unchanged. It was rejected for correctness, not cost. That bound assumes
products spread evenly across the four uint32 lanes after NEON's 32-bit
vaddq_u32 merge, and the even case already lands within 1,020 of
UINT32_MAX, while the masked residual load can add up to 16 products, or
1,040,400, into specific lanes. So lanes wrap before the widened reduce
sees them, and a correct bound would have to be derived per kernel from its
accumulator count and residual distribution. The narrow reduce needs none
of that: its bound is on the horizontal total, which does not depend on how
products land in lanes.
Recorded for whoever revisits this: on ARM the widening reduce is free
instruction-for-instruction. Cross-compiling with
clang++ --target=aarch64-linux-gnu -O2 emits addv/fmov w/ucvtf against
uaddlv/fmov x/ucvtf, three instructions either way. So the obstacle to a wider
band is the lane bound, not the reduce.
Nothing comparable supports that range regardless. Lucene caps its
scalar-quantized format at 1,024 dimensions and Elasticsearch caps dense
vectors at 4,096, both keeping a 32-bit accumulator safe by contract. Faiss's
QT_8bit_direct accumulates full-range bytes into 32-bit lanes with no widening
and carries the same theoretical limit. Qdrant quantizes to 0..127, lowering
the per-element cap to 16,129, and its raw uint8 metric still sums into i32.
The SQ8_SQ8 kernels reuse this helper, on main as much as here, so they now
take its result as uint32_t. Previously the AVX-512 one assigned it to int,
which wrapped past 33,025 once the helper stopped returning int, and the three
ARM ones to float, which lost exactness past 258. Note the SQ8_SQ8 choosers
have no dimension guard, unlike the uint8 ones, so the fence belongs with SQ8
index creation in #1007; on main nothing constructs an SQ8 index.
Split out of #1011 because none of this depends on the SQ8 metadata contract
that PR is changing, while all of it affects code reachable today. #1011
depends on this, through the helper above.
The regressions use all-255 bytes, the worst case, which makes the expected
value an exact integer, at dimensions 33,026 and 40,000 for the SIMD path and
66,052 for the fallback. The fallback test asserts the returned function
pointer, not just the distance: on a host with no uint8 SIMD tier the value
comparison would pass either way, but the pointer identity would not. The
existing UINT8 suites stop at dimension 128, which is why all of this went
unseen; being SIMD-versus-scalar comparisons they would also have agreed with
each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with delete
and stored the trailing norms through unaligned float casts, so measurements
taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 17, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
The uint8 kernels accumulate products or squared differences of bytes, so the
total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold that,
all of them on the plain int8/uint8 index paths that ship today.
* IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the scalar
UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB from
dimension 33,026, while the comment claimed support to 2^16. The
conditional was also dead: only int8_t and uint8_t instantiate these, both
1 byte, so it always selected int. ret_t is now 64-bit for every element
type, which also covers int8 at dimension 131,072. Kept signed so the
"1 - ip" in the wrappers stays signed arithmetic and cannot underflow. The
L2 comment still carried the old byte-counting rationale and is fixed.
* UINT8_InnerProductImp returned float on NEON and SVE, accumulating exactly
in integer lanes and then discarding it, exact only to dimension 258 since
2^24 / 65025 = 258.
* AVX-512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a signed
int, wrapping from dimension 33,026.
* L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned horizontal
reduce back into a signed int, so the distance went negative from the same
dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were already unsigned.
The accumulation was never the problem. The SIMD adds wrap modulo 2^32 and are
bit-exact, so the bit pattern was already correct; the top bit was being read
as a sign. An unsigned 32-bit reduce therefore costs nothing over the original
and is exact through dimension 66,051 = floor(UINT32_MAX / 65,025), twice the
old signed limit of 33,025. Verified: the AVX-512 object is byte-for-byte the
same size as before at 514,792, with the same 33 and 40 instructions for
residual 0 and 32.
Above 66,051 the choosers hand back the scalar kernel, which after the ret_t
change is exact to roughly dimension 2.8e14. One comparison at index creation,
reusing the "if (dim < 32) return ret_dist_func" idiom the choosers already
had, and it leaves every kernel untouched.
Three alternatives were tried and rejected, each on evidence:
* Widening the horizontal reduce unconditionally. Measured on an Ice Lake-SP
Xeon it costs 4 extra uops in the epilogue: +20% at dimension 32, +8-11%
across 55-200, +4-5% at 900-1024, on byte-identical loop code.
* A runtime branch selecting the width per call. Measured at +0.4 to 0.6 ns
per call, +15% at dimension 32, and it cost 31 of the 65 Cosine wrappers
their inlining, growing .text by 18.4%.
* Compile-time selection between two named wrappers, extending SIMD to a
second bound of 4 * 66,051. This one is free on the narrow path, verified:
the narrow wrappers stayed byte-identical and the out-of-line count
unchanged. It was rejected for correctness, not cost. That bound assumes
products spread evenly across the four uint32 lanes after NEON's 32-bit
vaddq_u32 merge, and the even case already lands within 1,020 of
UINT32_MAX, while the masked residual load can add up to 16 products, or
1,040,400, into specific lanes. So lanes wrap before the widened reduce
sees them, and a correct bound would have to be derived per kernel from its
accumulator count and residual distribution. The narrow reduce needs none
of that: its bound is on the horizontal total, which does not depend on how
products land in lanes.
Recorded for whoever revisits this: on ARM the widening reduce is free
instruction-for-instruction. Cross-compiling with
clang++ --target=aarch64-linux-gnu -O2 emits addv/fmov w/ucvtf against
uaddlv/fmov x/ucvtf, three instructions either way. So the obstacle to a wider
band is the lane bound, not the reduce.
Nothing comparable supports that range regardless. Lucene caps its
scalar-quantized format at 1,024 dimensions and Elasticsearch caps dense
vectors at 4,096, both keeping a 32-bit accumulator safe by contract. Faiss's
QT_8bit_direct accumulates full-range bytes into 32-bit lanes with no widening
and carries the same theoretical limit. Qdrant quantizes to 0..127, lowering
the per-element cap to 16,129, and its raw uint8 metric still sums into i32.
The SQ8_SQ8 kernels reuse this helper, on main as much as here, so they now
take its result as uint32_t. Previously the AVX-512 one assigned it to int,
which wrapped past 33,025 once the helper stopped returning int, and the three
ARM ones to float, which lost exactness past 258. Note the SQ8_SQ8 choosers
have no dimension guard, unlike the uint8 ones, so the fence belongs with SQ8
index creation in #1007; on main nothing constructs an SQ8 index.
Split out of #1011 because none of this depends on the SQ8 metadata contract
that PR is changing, while all of it affects code reachable today. #1011
depends on this, through the helper above.
The regressions use all-255 bytes, the worst case, which makes the expected
value an exact integer, at dimensions 33,026 and 40,000 for the SIMD path and
66,052 for the fallback. The fallback test asserts the returned function
pointer, not just the distance: on a host with no uint8 SIMD tier the value
comparison would pass either way, but the pointer identity would not. The
existing UINT8 suites stop at dimension 128, which is why all of this went
unseen; being SIMD-versus-scalar comparisons they would also have agreed with
each other wherever both wrapped.
Also fixes the uint8 spaces benchmark fixture, which paired new[] with delete
and stored the trailing norms through unaligned float casts, so measurements
taken from it can be trusted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 18, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 19, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 19, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 19, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 19, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 19, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
added a commit
that referenced
this pull request
Aug 19, 2026
…nput
QuantPreprocessor::quantize could execute undefined behaviour for input the
API accepts. Three paths, all ending at the same conversion to a byte, and
all reachable with entirely finite components.
The scale was derived in FP32, so for [-FLT_MAX, +FLT_MAX] the range
overflowed: max - min became inf, delta inf, inv_delta 0, and the
per-element product inf * 0 = NaN, whose conversion to an integer is
undefined. UBSan: "-nan is outside the range of representable values of
type 'unsigned char'". The existing diff == 0 guard covers equal values,
not overflow of the subtraction. (MOD-17528)
With WithNorm, centering is an FP32 subtraction, so finite input against a
finite mean reaches inf before any range is computed: FLT_MAX against mean
-FLT_MAX centers to 6.8e38. Widening the range does not help, because the
value is already lost upstream.
And delta is stored as FP32, so (float)(diff / 255) underflows to zero
while diff itself is nonzero, which leaves 1/delta inf and scales the
minimum element, numerator exactly zero, to 0 * inf.
find_min_max now guarantees that both endpoints are finite and ordered, and
everything else follows from that. The guarantee lives there rather than at
the call site because that is where it can be broken: the WithNorm branch
creates the inf itself, from two valid operands, and the plain branch passes
through whatever the input holds. Both endpoints get a two-sided clamp,
since for an all-+inf vector inf <= inf passes the order check and a
one-sided std::max would leave min at +inf. Clamping to the float range is
not merely defensive either: min is stored as an FP32 field, so a non-finite
endpoint could not be represented under any arithmetic.
With that established, quantize needs one delta comparison, and inv_delta
stays double. Not for precision, which needs only +/-0.5 in 255, but because
an FP32 reciprocal overflows for a subnormal delta: [0, 7e-37] gives delta
2.7e-39, whose FP64 reciprocal is finite and correctly maps the top element
to 255.
The per-element bound and rounding are written by hand. That replaces
std::round, an out-of-line libm call at this translation unit's baseline
that ran once per element: bounding first makes adding 0.5 and truncating
equivalent for non-negative values. Measured at -O3 it is 8 instructions
against 9 for std::clamp plus std::round's call.
Scope, stated precisely, because it is narrower than it might look:
* The byte conversion is defined for all finite components. That is the
goal and it is met.
* The stored min and delta are finite with delta positive. The *sums* are
not covered: they are accumulated in FP32 over the input values, so
[-FLT_MAX, +FLT_MAX] stores sum_squares as inf even though this
function's own arithmetic is now well defined. Separate problem,
separate change.
* Non-finite components are unsupported, and nothing here is tested
against them. std::minmax_element requires its comparison to induce a
strict weak ordering, and floating-point < is not one once a NaN is
present: incomparability must be transitive, yet 1.0 is incomparable
with NaN and NaN with 2.0 while 1.0 and 2.0 are comparable. Violating
that precondition is undefined behaviour inside the algorithm, before
any range exists, so no assertion about the outcome would be portable,
including a weak one about metadata finiteness. UBSan reporting nothing
does not establish otherwise. Rejecting non-finite components at the
public ingestion boundary is the actual fix and belongs in its own
change; nothing in VecSim does it today.
The order check on the endpoints stays, as an invariant guard rather than a
NaN policy: everything downstream is written assuming min <= max, and
asserting that once per vector costs less than reasoning about whether it
can be violated.
This matches how comparable systems handle it. Lucene validates vector
components and throws on NaN or infinity, and Elasticsearch rejects NaN,
infinity and magnitudes that overflow before delegating to Lucene. Faiss
guards only an exactly zero range and assumes finite input otherwise, with
the same float-to-integer concern at its final cast. Qdrant gets defined
bytes from Rust's saturating cast, which C++ does not have, and can still
store an unusable scale.
Tests: the MOD-17528 reproduction, the WithNorm centering case, and a
table-driven matrix over finite input only: constant vectors positive
negative and zero, a single element, a subnormal but representable delta, a
range that underflows and collapses, and the full FP32 range. Expected bytes
and scale metadata are asserted rather than a range check, which is vacuous
for uint8_t, and every expectation was derived by simulating the pipeline
rather than predicted. The parameterized test is named for what it checks,
ScaleMetadataAndBytesAreAsExpected, since the sums are out of scope.
Metadata meaning is unchanged: the sums are still FP32 over the input
values. Making them exact integer sums over the quantized bytes is a
storage-contract change that has to move together with every kernel that
reads them, and stays in #1011.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
force-pushed
the
dor-forer-MOD-17526-sq8-exact-metadata
branch
from
August 19, 2026 19:07
729f621 to
810e973
Compare
dor-forer
changed the title
dor-forer
force-pushed
the
dor-forer-MOD-17526-sq8-exact-metadata
branch
from
August 20, 2026 07:50
ae7feb9 to
f3bf3d5
Compare
dor-forer
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f3bf3d5. Configure here.
dor-forer
enabled auto-merge
August 20, 2026 10:36
The stored sum and sum_squares described the input values x[i], but every kernel
term is written in terms of the reconstruction x_r[i] = min + delta * a[i]. The
two differ by the quantization error, about 0.4% of ||x||^2, which is larger than
the distance between two similar vectors. So L2 came back wrong, and negative:
for two near-duplicate vectors at dimension 128 the reconstruction distance is
2.93e-04 and the kernels returned -1.49e-01.
The existing algebra is already exact once the sums describe x_r:
IP = min1*sum2 + min2*sum1 - dim*min1*min2 + delta1*delta2*sum(a[i]*b[i])
= IP(x_r, y_r) iff sum = sum(x_r[i]) = dim*min + delta*sum(a[i])
L2 = sum_sq_x + sum_sq_y - 2*IP
= L2(x_r, y_r) iff sum_sq = sum(x_r[i]^2)
so quantize() now accumulates the quantized bytes as exact integers and derives
both sums from them in double before storing FP32:
sum = dim*min + delta*q_sum
sum_squares = dim*min^2 + 2*min*delta*q_sum + delta^2*q_sum_squares
No kernel changes. The blob layout, the slot count and the slot types are all
unchanged, and the metadata stays FP32 for every metric, so nothing downstream
has to know this happened. SUM has exactly one reader, the inner product algebra
above, which is what makes a metadata-only fix sufficient.
Note the query blob keeps sums over the input, because a query is not quantized:
the asymmetric distance is sum(x_r^2) + sum(y^2) - 2*sum(x_r*y). Storage and
query now describe different quantities, and three test assertions that compared
one against the other are updated. They only ever agreed because both used to be
the input sum, which is the same confusion this change fixes in the product code.
Tests: L2 must be non-negative and match a double-precision reference over the
reconstruction, on near-duplicate vectors across five dimensions and on both the
scalar and dispatched kernels. Near-duplicates are the case that exposes this,
since the true distance is small enough for the mismatch to dominate.
Self-distance is asserted near zero rather than exactly zero: the two sides of
sum_sq_x + sum_sq_y - 2*IP are computed by different routes and round
differently.
Left for follow-up, all pre-existing: FP32 cancellation for vectors sharing a
large offset, exact-zero self-distance, and the MAX_EXACT_DIM comment claiming a
bound the code does not have.
Verified on Graviton4: test_spaces 1549/1549 and test_components 51/51, both
suites unfiltered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The byte accumulators inside quantize() are local counters, not stored fields: the metadata slots remain four FP32 values and the blob layout is unchanged. But the squares counter was uint32_t, and each byte contributes up to 255^2, so the total passes UINT32_MAX just above dimension 66051. A wrapped counter feeds the double expansion that fills sum_squares, so the stored norm and every L2 distance built on it would be wrong rather than merely imprecise. It also fits the documented 2^16 limit by under 1%. The four chains and the combined total are now uint64_t, in the product code and in both test mirrors of the quantizer. The class documentation still defined x_sum and x_sum_squares as sums over the input values, which is what this branch changed. That prose is where the defect came from: it wrote the asymmetric L2 identity over x while the kernel's inner product term is over the reconstruction. Both are now written in terms of x_r, and the symmetric inner product note records that recovering the quantized sum from the stored sum is exact rather than approximate, which is what makes a metadata-only fix sufficient. Tests: The regression tolerance was 1e-3 * max(1.0, expected), which collapses to a flat 1e-3 because the distances asserted run from 7.7e-7 to 2.2e-3. At dimension 4 that is over a thousand times the value being pinned, so a kernel returning zero would pass, and the old metadata cleared the threshold by only 1.5x. The error floor is FP32 cancellation in sum_sq_x + sum_sq_y - 2*IP, which scales with the norms and not with the distance, so the bound is now 8 * FLT_EPSILON * (norm_x + norm_y). EXPECT_GE(got, 0.0f) asserted a property this change does not provide. The residual cancellation is about 1e-7 of the norm, and two identical blobs have a true distance of zero, so self distance lands just below it. The bound is now the noise floor rather than zero, and the limitation is stated where it is asserted. Added SQ8_FP32_L2_matches_reconstruction_against_float_query. The asymmetric path is how the defect stayed hidden: its tests run dimensions 1, 5, 7 and 15 against an absolute tolerance of 0.01, and 0.4% of the norm at dimension 5 is about 0.007, which fits underneath. The new test runs near duplicates out to dimension 512 against a double reference, on both the scalar and dispatched kernels. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
force-pushed
the
dor-forer-MOD-17526-sq8-exact-metadata
branch
from
August 20, 2026 10:41
d5b467e to
e96910f
Compare
ethanglaser
pushed a commit
to ethanglaser/VectorSimilarity
that referenced
this pull request
Aug 21, 2026
RedisAI#1014) * fix(uint8): make the integer accumulators exact, with a dim fallback The uint8 kernels accumulate products or squared differences of bytes, so the total reaches 255 * 255 * dim = 65025 * dim. Four paths could not hold that, all of them on the plain int8/uint8 index paths that ship today. * IP.cpp / L2.cpp: ret_t for a 1-byte element type was int, so the scalar UINT8_InnerProduct and UINT8_L2Sqr executed signed-overflow UB from dimension 33,026, while the comment claimed support to 2^16. The conditional was also dead: only int8_t and uint8_t instantiate these, both 1 byte, so it always selected int. ret_t is now 64-bit for every element type, which also covers int8 at dimension 131,072. Kept signed so the "1 - ip" in the wrappers stays signed arithmetic and cannot underflow. The L2 comment still carried the old byte-counting rationale and is fixed. * UINT8_InnerProductImp returned float on NEON and SVE, accumulating exactly in integer lanes and then discarding it, exact only to dimension 258 since 2^24 / 65025 = 258. * AVX-512 reduced 16 int32 lanes with _mm512_reduce_add_epi32 into a signed int, wrapping from dimension 33,026. * L2_AVX512F_BW_VL_VNNI_UINT8 and L2_NEON_UINT8 read an unsigned horizontal reduce back into a signed int, so the distance went negative from the same dimension. L2_NEON_DOTPROD_UINT8 and L2_SVE_UINT8 were already unsigned. The accumulation was never the problem. The SIMD adds wrap modulo 2^32 and are bit-exact, so the bit pattern was already correct; the top bit was being read as a sign. An unsigned 32-bit reduce therefore costs nothing over the original and is exact through dimension 66,051 = floor(UINT32_MAX / 65,025), twice the old signed limit of 33,025. Verified: the AVX-512 object is byte-for-byte the same size as before at 514,792, with the same 33 and 40 instructions for residual 0 and 32. Above 66,051 the choosers hand back the scalar kernel, which after the ret_t change is exact to roughly dimension 2.8e14. One comparison at index creation, reusing the "if (dim < 32) return ret_dist_func" idiom the choosers already had, and it leaves every kernel untouched. Three alternatives were tried and rejected, each on evidence: * Widening the horizontal reduce unconditionally. Measured on an Ice Lake-SP Xeon it costs 4 extra uops in the epilogue: +20% at dimension 32, +8-11% across 55-200, +4-5% at 900-1024, on byte-identical loop code. * A runtime branch selecting the width per call. Measured at +0.4 to 0.6 ns per call, +15% at dimension 32, and it cost 31 of the 65 Cosine wrappers their inlining, growing .text by 18.4%. * Compile-time selection between two named wrappers, extending SIMD to a second bound of 4 * 66,051. This one is free on the narrow path, verified: the narrow wrappers stayed byte-identical and the out-of-line count unchanged. It was rejected for correctness, not cost. That bound assumes products spread evenly across the four uint32 lanes after NEON's 32-bit vaddq_u32 merge, and the even case already lands within 1,020 of UINT32_MAX, while the masked residual load can add up to 16 products, or 1,040,400, into specific lanes. So lanes wrap before the widened reduce sees them, and a correct bound would have to be derived per kernel from its accumulator count and residual distribution. The narrow reduce needs none of that: its bound is on the horizontal total, which does not depend on how products land in lanes. Recorded for whoever revisits this: on ARM the widening reduce is free instruction-for-instruction. Cross-compiling with clang++ --target=aarch64-linux-gnu -O2 emits addv/fmov w/ucvtf against uaddlv/fmov x/ucvtf, three instructions either way. So the obstacle to a wider band is the lane bound, not the reduce. Nothing comparable supports that range regardless. Lucene caps its scalar-quantized format at 1,024 dimensions and Elasticsearch caps dense vectors at 4,096, both keeping a 32-bit accumulator safe by contract. Faiss's QT_8bit_direct accumulates full-range bytes into 32-bit lanes with no widening and carries the same theoretical limit. Qdrant quantizes to 0..127, lowering the per-element cap to 16,129, and its raw uint8 metric still sums into i32. The SQ8_SQ8 kernels reuse this helper, on main as much as here, so they now take its result as uint32_t. Previously the AVX-512 one assigned it to int, which wrapped past 33,025 once the helper stopped returning int, and the three ARM ones to float, which lost exactness past 258. Note the SQ8_SQ8 choosers have no dimension guard, unlike the uint8 ones, so the fence belongs with SQ8 index creation in RedisAI#1007; on main nothing constructs an SQ8 index. Split out of RedisAI#1011 because none of this depends on the SQ8 metadata contract that PR is changing, while all of it affects code reachable today. RedisAI#1011 depends on this, through the helper above. The regressions use all-255 bytes, the worst case, which makes the expected value an exact integer, at dimensions 33,026 and 40,000 for the SIMD path and 66,052 for the fallback. The fallback test asserts the returned function pointer, not just the distance: on a host with no uint8 SIMD tier the value comparison would pass either way, but the pointer identity would not. The existing UINT8 suites stop at dimension 128, which is why all of this went unseen; being SIMD-versus-scalar comparisons they would also have agreed with each other wherever both wrapped. Also fixes the uint8 spaces benchmark fixture, which paired new[] with delete and stored the trailing norms through unaligned float casts, so measurements taken from it can be trusted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(uint8): accumulate in chunks instead of capping the SIMD dimension The previous commit kept the 32-bit SIMD accumulators and had the choosers hand back the scalar kernel past the dimension where those accumulators stay exact. That works but gives up SIMD entirely for large-dimension indexes, and the bound it relied on was only sound for the even lane distribution. Instead, split each uint8 kernel into an Imp that returns its raw integer total and two wrappers over it: - the plain wrapper, unchanged in behaviour, for dimensions up to UINT8_CHUNK_ELEMENTS (65,536) - a chunked wrapper that calls Imp once per chunk and folds the per-chunk totals in 64 bits The choosers pick between them once per index, so the plain kernel carries no branch. 65025 * 65536 = 4,261,478,400 <= UINT32_MAX, so each chunk's 32-bit total is exact, and because every contribution is non-negative no individual accumulator lane can exceed the chunk total either. That is the entire correctness argument: no reasoning about how work spreads across lanes. The first chunk absorbs the residual, which leaves every later chunk a whole multiple of the kernel's step and so matches the residual-0 precondition. On SVE the vector length is a runtime value, so that split is computed in the wrapper rather than at compile time. Covers all five kernel families (AVX512 VNNI, NEON, NEON DOTPROD, SVE, SVE2) for L2, inner product and cosine. SQ8_SQ8 calls the helper directly rather than through a uint8 chooser, so it does not gain chunking; it is capped well below the chunk size by its uint32 metadata slot, and that fence belongs with SQ8 index creation. Also marks each Imp static and always_inline. always_inline keeps the plain wrappers byte-identical now that Imp has more call sites: without it GCC outlines Imp and the plain wrapper loses its inlining too. static removes a latent ODR problem, since the NEON and NEON DOTPROD headers define the same Imp name with different bodies and both are compiled into an ARM build. Replaces the fallback test with one that checks the dispatched kernel agrees exactly with the 64-bit scalar kernel across the chunk boundary, and one that checks the chooser actually switches families using two dimensions with the same residual. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(uint8): sweep every residual past the chunk boundary, and every tier The boundary test sampled seven dimensions and went through the generic dispatcher. Two gaps followed from that. First, only seven of the 64 residual instantiations were covered past the boundary, so a seam between the residual-bearing first chunk and the residual-0 chunks after it could have survived in the other 57. 65,600 and 196,608 are both multiples of 64, so base + r has residual r; sweeping r over 0..63 at both bases covers every shape one chunk past the boundary and again three chunks past it. A ramp against all-255 is position sensitive, so a seam that skips or double-counts elements changes the total rather than cancelling out, and the total stays above UINT32_MAX so the 64-bit fold is under test throughout. Second, the dispatcher only ever returns the best tier the host supports, so on a machine with SVE the NEON and NEON_DOTPROD chunked kernels never ran at all. The new tier test calls each compiled-in chooser directly, still gated on the CPU supporting it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(uint8): report which tiers the per-tier test actually exercised A tier the CPU does not support is skipped by a plain if, so on a host with no uint8 SIMD at all the test passed without checking a single SIMD kernel, and the log gave no way to tell. Record and print the tiers covered per dimension, so a green run states what it proved rather than leaving it to be inferred from the host's feature flags. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(uint8): correct the linkage comment, the collision is live on gcc 12.3 The comment claimed both copies of UINT8_InnerProductImp are fully inlined at -O2 so nothing collides, and that static was therefore defensive. Measured on an aarch64 host with gcc 12.3: not inlined. Both objects emit one weak COMDAT symbol per residual under the same mangled name, the linker keeps a single body, and on main the plain NEON inner product and cosine wrappers branch into the NEON_DOTPROD body and execute udot. That faults on a core with asimd but without asimddp, which includes Neoverse-N1 and Graviton2. x86-64 gcc 13/14 and aarch64 clang 18 do inline it and do not collide, which is why the earlier check came back clean and why this cannot be left to the toolchain. static is load-bearing on at least one shipping compiler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * perf(uint8): stop the chunked wrapper pessimising its first chunk, and cheapen the IP epilogue Two findings from benchmark runs on an Ice Lake-SP host and an ARM host, both measured rather than inferred. First, the chunked wrapper was 8-9.5% slower per element than the plain kernel on the stretch its first chunk covers. Cause: the first chunk's length was a compile-time constant, so its loop had a compile-time trip count, and GCC then split that loop's accumulator and copied it in and out every 64 elements. The inner product loop went 12 instructions with no register moves to 13 with two; L2 went 14/0 to 15/2. The later-chunks loop was always fine. Fixed by two changes to the chunked wrappers only, leaving the plain path alone: - the first chunk's length is now a runtime min against the dimension, so the trip count is not a constant. Both loops are back to 12/0 and 14/0, matching the plain kernels exactly, at every residual. The min also makes the chunked wrapper correct at any dimension rather than only past the chunk size. - the residual-0 chunks now go through one out-of-line copy of the kernel instead of being inlined into all 64 chunked wrappers. One call per 65,536 elements is unmeasurable, and it drops the AVX512 inner product family from 41,267 to 34,229 bytes of text, against 14,735 for the plain family alone. Second, the inner product epilogue. Converting the unsigned total to float and subtracting in float costs one instruction more than main's integer subtract plus a single signed convert, and rounds twice instead of once; it measured about 1% slower across all four IP benchmark groups. Restored the integer form, which is also what INT8_InnerProduct already does. The cast to int64_t is required because ret_t is unsigned for uint8, so 1 minus the total would otherwise wrap. Applied to the scalar kernel too, so scalar and SIMD stay bit-identical, which the exactness tests assert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(uint8): spell out the inner product's integer subtract and conversion The epilogue read as `return 1 - static_cast<int64_t>(...)` from a function declared to return float, which relies on an implicit narrowing conversion to carry the intent and reads like a type error. Same arithmetic, written out: hold the total in a signed local, subtract in integer, convert once explicitly. GCC folds the two forms to the same code, so this is readability only. The rationale now appears once per header on the plain wrapper rather than on every wrapper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(uint8): collect the kernel notes into one block per file The rationale had spread to a block above almost every definition, and some of it was repeated across headers and across the choosers. Collected into a single block at the top of each kernel header, with spaces.h left as the one place that carries the chunk-size argument, and the three-line reduce notes cut to one line. The chooser note went from three copies per file to one. Comment lines across the eight uint8 kernel headers: 249 to 161. Codegen is byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Add shared uint8 chunked-accumulation driver Extracts the chunked-accumulation formula duplicated across the eight uint8 SIMD kernel headers into a single templated driver, so each kernel's 32-bit per-chunk total stays exact regardless of dimension. * Migrate SVE uint8 IP/L2 kernels to the shared chunking driver Replace the hand-written chunked-accumulation logic in IP_SVE_UINT8.h and L2_SVE_UINT8.h with adapters (UINT8_IPChunkKernel_SVE, UINT8_L2ChunkKernel_SVE) over spaces::uint8_chunked_total, using granule() = 4 * svcntb() for SVE's runtime block size. * Migrate IP kernels to shared uint8_chunking driver Replace local UINT8_InnerProductChunkedImp function templates in three inner-product kernel headers with adapter structs that call the shared chunked-accumulation driver in uint8_chunking.h. Reduces duplication across AVX512F_BW_VL_VNNI_UINT8, NEON_UINT8, and NEON_DOTPROD_UINT8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Add architecture-independent test for uint8 chunked-accumulation driver Exercises spaces::uint8_chunked_total directly with a mock kernel so the chunk-tiling invariants (exact tiling, granule-multiple steps, chunk-size cap) are verified on any host, not only through whatever SIMD kernel a given CPU happens to support. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Assert the granule precondition in the uint8 chunking driver Kernel::granule() must be in (0, UINT8_CHUNK_ELEMENTS] for the chunking arithmetic to hold; assert it instead of dividing by zero or silently underflowing chunk - tail. Documents the precondition that the invariants already relied on. * Add value-based end-to-end check to the uint8 chunked-driver test The tiling offset check compared a recorded offset to the cumulative sum of prior recorded lengths, both derived from the same driver-advanced pointer, so it could not fail on its own. Keep it (it still guards the coupling between the length passed to the kernel and the pointer advance) and add a real correctness check: the mock kernel now returns the sum of the bytes in the slice it was handed, read from a position-dependent fill, and the driver's total is compared against an independent sum over the whole buffer. That catches skipped, duplicated or mis-sized chunks that the offset check cannot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(uint8): make a zero-tier run skip, let CI demand its tier, and enforce the granule at compile time Three review points, all of which made a green result mean less than it looked. A run that exercised no SIMD tier now reports skipped rather than passed. On a host without the relevant instruction set the per-tier test executed no chunked kernel at all, and reporting that as a pass reads as coverage that does not exist. Hardware-specific CI can now name the tier it exists to cover by setting VECSIM_REQUIRE_UINT8_TIER to AVX512F_BW_VL_VNNI, SVE2, SVE, NEON_DOTPROD or NEON. The requirement is checked before the skip, so a mislabeled or silently downgraded runner fails instead of quietly skipping. Verified both ways: without the variable the test skips on this host, with it set the test fails and names the tier it could not reach. The granule precondition was guarded only by assert, which disappears under NDEBUG, so it protected nobody in a release build. granule() is now constexpr in the six fixed-width adapters, and the driver static_asserts the bound whenever the adapter can express it as a constant. Verified that granule 0 and granule 70000 both fail to compile under -DNDEBUG while 64 compiles. SVE keeps the runtime assert because its granule depends on the vector length and genuinely cannot be constant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(uint8): assert worst-case inputs against an independent 64-bit oracle, and let CI demand its tier Closes the two coverage gaps that this PR's own changes created, rather than deferring them. Independent oracle. Every existing uint8 kernel test used the scalar kernel as its oracle, which is a different code path but not an independent one. This series changed the scalar and SIMD inner product epilogues together so they would stay bit-identical, so a test asserting only scalar == SIMD cannot catch that shared convention being wrong. The new test derives its expectation from the inputs alone in 64-bit integer arithmetic and asserts the scalar kernel and every available SIMD tier against it on the same footing. Demonstrated non-vacuous: mutating the scalar epilogue from 1 - ip to ip - 1 leaves the residual-sweep test passing and fails only the oracle test. Worst-case overflow. The residual sweep used a ramp against all-255, averaging roughly half the maximum accumulator load, and the worst case appeared only at seven sampled dimensions through the dispatched tier. The new test sweeps every residual with all-255 against all-255, which puts 65,025 per element into the inner product accumulator, and all-255 against all-0, which does the same for L2. It does so at two bases: 65024+r, which stays under the chunk size and so drives the plain kernel's 32-bit reduce to about 4.23e9, just under UINT32_MAX; and 131072+r, whose total near 8.5e9 only a 64-bit fold can carry. A ramp pair is kept alongside because constant data lets a gap and an overlap of equal size cancel. The test asserts the worst-case totals actually exceed UINT32_MAX, so it cannot quietly stop exercising the fold. Tier discovery is now shared between this test and the per-tier test, so a tier cannot be covered by one and missed by the other. CI. task-unit-test.yml takes a require-uint8-tier input, exported as VECSIM_REQUIRE_UINT8_TIER. The dedicated ARM job requires SVE2, which r8g Graviton4 has, and the coverage job requires AVX512F_BW_VL_VNNI on both suite runs, which c7i Sapphire Rapids has. Those two jobs now fail rather than skip if the hardware they exist to cover is absent. Generic-CPU jobs leave it unset and skip as before. Also documents why SVE keeping only a runtime assert is acceptable: the architecture bounds an SVE vector to 16 to 256 bytes, so its granule is 64 to 1024, far below the 65,536 limit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): stop an unset tier requirement from failing every job The previous commit wired VECSIM_REQUIRE_UINT8_TIER into task-unit-test.yml as `${{ inputs.require-uint8-tier }}`. When that input is unset, GitHub Actions still puts the variable in the environment with an empty value, so getenv returned a pointer to "" rather than nullptr and the requirement fired asking for a tier named empty string. That failed the per-tier test on every job that did not name a tier, which is the sanitizer, jammy, alpine and macos jobs. Both PRs went red. Two changes. The workflow wiring is reverted: arm.yml, coverage.yml and task-unit-test.yml go back to what they were, so the requirement is opt-in for manual and hardware runs, which is how it was actually used to validate this work on Graviton4 and Sapphire Rapids. And the test now treats an empty value as unset, so re-wiring it later cannot reintroduce the same failure. Verified all three cases: unset skips, set but empty skips, set to a tier this host lacks still fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(uint8): widen the cosine norm accumulator and make the AVX512 reduce unsigned Both defects were found by @lerman25 reviewing MOD-17527, and both are real. The cosine norm. IntegralType_ComputeNorm accumulated into a signed int, so with 65,025 per uint8 element the total passed INT32_MAX from dimension 33,026. That is the norm the cosine preprocessor writes for every stored vector and every query, so a wrong value reached the kernels before they ran: dimension 65,537 produced a NaN norm and 66,052 produced 252.99 instead of about 65,536.49. Widened to uint64_t. int8 had the same shape with a higher bound, overflowing from 133,153. The kernel tests never caught this because they append the norm by hand, so the production path was untested. Added two tests that go through it: VecSim_Normalize on an all-255 uint8 vector at 33,025, 33,026, 65,537 and 66,052, and a uint8 cosine brute force index storing and querying itself at 65,537, where the self-distance must be about zero. The AVX512 reduce. static_cast<uint32_t>(_mm512_reduce_add_epi32(sum)) casts after the reduction, and GCC implements that intrinsic as a chain of signed __v8si ops ending in a scalar `int + int`. A chunk total reaches 65025 * 65536, about 4.26e9, roughly twice INT32_MAX, so the addition overflows inside a single chunk. The wrapped bits are the ones we want, which is why every equality test passes, but the addition is signed-overflow UB. My comment claiming the adds were all vector operations was wrong about that last step. Replaced with a fold that zero-extends the 16 lanes to 64 bits before summing, in both the IP and L2 AVX512 kernels. Costs 2 instructions per call on the plain path, with the SIMD loop unchanged. NEON and SVE are unaffected: vaddvq_u32 and svaddv_u32 are genuinely unsigned. Verification status: the norm fix is verified directly, dimension 65,537 now gives 65,280.5 rather than NaN. The AVX512 fold is compile-verified and cost-measured only. This dev box has no AVX512, so its numeric result and the original UB both need a run on AVX512 hardware, ideally under -fsanitize=signed-integer-overflow at dimensions 33,025, 33,026 and 65,536 as suggested in review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(uint8): replace chunked accumulation with a scalar fallback above the exact dimension The chunked design made the uint8 kernels exact at any dimension, but it cost a shared driver, eight chunked wrappers, eight adapters, eight out-of-line chunk helpers, and a large amount of architecture-specific test machinery, all to serve dimensions above 66,051 that do not occur in real workloads. Embeddings run 384 to 4096. Replaced with a bound and a fallback. UINT8_MAX_EXACT_SIMD_DIM = UINT32_MAX / (UINT8_MAX * UINT8_MAX) = 66,051 Derived from the types rather than written as a literal, because the bound is a property of uint8 accumulating into uint32. At 66,051 the worst-case total is 4,294,966,275, which fits with 1,020 to spare; at 66,052 it does not. The three uint8 dispatchers return the scalar kernel above the bound, once per index, so no distance computation pays for the check. Kept, because none of it depends on chunking: - the widened scalar accumulator, which is what makes the fallback correct - uint32_t SIMD results and the integer-subtract inner product epilogue. These are load-bearing at this bound, not hygiene: the total passes INT32_MAX from dimension 33,026, so a signed result wraps negative across a 33,026-wide band that stays on SIMD - the widened 64-bit AVX-512 fold. Also load-bearing for the same reason: GCC implements _mm512_reduce_add_epi32 as signed vector ops ending in a scalar int + int, which overflows across that same band. It cannot be dropped in favour of the dispatcher bound because the SQ8_SQ8 kernels call the helper directly and never pass through a uint8 chooser - static linkage separating the NEON and NEON_DOTPROD helpers, which fixes an unrelated udot fault on cores without asimddp - the cosine norm accumulator widened to uint64_t Removed: uint8_chunking.h, every _Chunked wrapper, every ChunkKernel adapter, every FullChunk helper, and the multi-chunk tests. Tests now pin two boundaries. The signed boundary, 33,025 against 33,026, where both sides stay on SIMD and 33,026 is what exercises the widened fold. And the dispatcher boundary, 66,051 against 66,052, where the second must come back as the scalar kernel by name. The oracle test sweeps every residual at 33,024+r and 65,984+r with worst-case inputs, asserting the upper base exceeds INT32_MAX and stays within UINT32_MAX, so it cannot drift off the case it exists for. Verified on an AVX-512 host: 472/472 pass, and the per-tier and oracle tests both report AVX512F_BW_VL_VNNI at every boundary dimension rather than passing silently. A negative control demanding SVE2 on that host fails as designed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trim the uint8 comments back to the density of the surrounding kernels The comment blocks added for the accumulator work had grown past what the rest of the spaces directory carries. The untouched int8 siblings sit at 18 to 23 percent comment lines; several of these files had reached 28 to 51. Removed as duplication rather than as explanation: - compute_norm.h's seven-line accumulator note, down to two. The measured symptoms belong in the pull request, not in the header; the header needs the rule. Its in-loop promotion note went too, since the explicit static_cast says it and the inherited wording had become false. - The six-line fold rationale in both AVX512 kernels, down to three. The arithmetic it restated already lives in spaces.h. - The reciprocal paragraph in spaces.h explaining the AVX512 fold, which the fold now explains at the fold. - The static and always_inline note in six of eight kernels, where it is only hygiene. Kept in the two NEON inner product headers, which is where the names actually collide. Kept the udot linkage note, the bound's derivation, and the INT32_MAX at 33,026 fact, which is what justifies uint32_t and is not visible from any single kernel. Moved one comment rather than dropping it: the note about the epilogue's int64_t cast now sits at each epilogue instead of in a header block ninety lines away. Without that cast the subtraction is unsigned and wraps, so it needs to be readable where someone might simplify it. No code changed. Verified comment-only mechanically, and rebuilt to confirm the tested objects postdate the edits: 578/578. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cap uint8 SIMD at the signed 32-bit bound instead of widening the reduce The bound was UINT32_MAX / 65025 = 66,051, which required the AVX512 kernels to replace _mm512_reduce_add_epi32 with a zero-extend-then-reduce-in-64-bit sequence, because GCC implements that intrinsic as signed vector operations ending in a scalar int + int. Keeping SIMD exact through the full unsigned range therefore cost a wider epilogue on every call, at every dimension, to serve dimensions from 33,026 to 66,051 that no workload reaches: embeddings run 384 to 4096. The bound is now INT32_MAX / 65025 = 33,025, still derived from the types. Both AVX512 epilogues go back to the single intrinsic, identical to main. Six dispatchers carry the guard rather than three. The SQ8_SQ8 kernels call the shared UINT8_InnerProductImp directly and never pass a uint8 chooser, so with the widened fold gone they would have been the one path still able to wrap a signed reduce. They have their own dimension-aware choosers, so they take the same guard. Their scalar fallback accumulates in float, imprecise past 2^24 but well defined, which is the better of the two failure modes. NEON and SVE reduce genuinely unsigned and would be exact to 66,051 either way. The bound is uniform across architectures on purpose: a per-architecture bound would make the dispatcher's answer to "is this dimension on SIMD" depend on the host, so two machines would disagree about which kernel serves dimension 50,000. Tests collapse to one boundary, since 33,025/33,026 is now both the signed-reduction boundary and the dispatcher boundary. The oracle sweep runs 32,960 to 33,025 contiguously, covering every residual and ending exactly at the cap, and asserts the worst case at the cap is both within INT32_MAX and within one element's product of it, so it fails if it ever drifts off the boundary rather than quietly testing an easier dimension. That assert immediately caught a latent bug in the test itself: each dimension's cosine norm is written at vec + dim, which is payload for every larger dimension, so with a contiguous sweep dimension 32,960's norm corrupted 65 bytes of dimension 33,025's all-255 vector and the total came up 2,753,904 short. The old two-base sweep could not see this because both sides of the comparison read the same corrupted array. Both tier-aware tests now save and restore those bytes. Verified on Xeon Platinum 8375C: 1036/1036, with AVX512F_BW_VL_VNNI reported at all five boundary dimensions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Reduce the uint8 fix to the changes the cap actually requires With SIMD capped at the signed 32-bit bound, main's kernel arithmetic is already correct: ARM does `int32_t result = vaddvq_u32(total_sum)` and x86 does `_mm512_reduce_add_epi32`, and the largest total the cap admits is 2,147,450,625, which fits int32_t. So none of the kernels needed a type change, and the uint32_t returns, the int64_t casts in the inner product epilogues, the L2 Imp/wrapper split and the SQ8 call site changes were all serving the earlier 66,051 design rather than this one. They are reverted to main, along with the unrelated benchmark fixes. What remains is what the cap requires: - spaces.h: the bound, derived from the types. - Six choosers return the scalar kernel above it, once per index. Three for uint8 and three for SQ8_SQ8, because the SQ8 kernels call the shared uint8 helper directly and no uint8 chooser sees those calls. - ret_t is `long long` for every element type rather than `int` for one-byte ones. The scalar kernel is the fallback above the cap, so it has to be exact there. Keeping it signed means the wrappers keep main's `1 - ip` form. - compute_norm.h accumulates into `long long`. The norm is written by the cosine preprocessor for every stored vector and query regardless of which kernel runs, so this is independent of the cap: at dimension 65,537 the norm came back NaN. - Internal linkage on three inner product helpers. The linkage fix covers three headers, not the two NEON ones. IP_SVE_UINT8.h is compiled into both SVE.cpp (-march=armv8-a+sve) and SVE2.cpp (-march=armv9-a+sve2), so one mangled name carried two independently generated bodies there too. That pattern is not uint8-specific and affects 14 SVE headers across every data type; measured benign today, since the shared symbols use identical instruction sets, and tracked as MOD-17759 rather than fixed here. Also dropped the independent-oracle test. Its justification was that this PR changed the scalar and SIMD epilogues together, so asserting scalar == SIMD would be blind to a wrong shared convention. Neither epilogue is changed any more, so that reasoning no longer holds. Its one unique contribution, pinning the bound arithmetically rather than trusting the constant, survives as two static_asserts in the boundary test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correct the bound's comment: it named the wrong reason and overstated one claim Three fixes, all to text I wrote: The comment justified bounding at the signed limit only through GCC's _mm512_reduce_add_epi32. That is a supporting detail. The reason is that every kernel already holds its total in a signed 32-bit int, on ARM as `int32_t result = vaddvq_u32(...)` and on x86 as the reduce's int return, and this change leaves them alone. It claimed NEON and SVE "would be exact to 66,051 either way". That is false. They reduce with unsigned intrinsics but store the result into int32_t, so reaching 66,051 there would need a type change too. It asserted that widening the reduce "costs measurably at the dimensions that occur in practice". The measurement behind that is from an earlier revision of this branch and was not repeated against this head, so the claim does not belong in a code comment stated as fact. The remaining argument, that the extra range serves no real workload, stands on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * State 33,025 in the bound's comment instead of leading with the number we rejected The comment explained at length why the bound is not at the unsigned limit, so 66,051 appeared twice directly above a constant whose value is 33,025, and reading it left the impression that the constant was the larger number. The value it defines is now stated plainly, with the arithmetic that fixes it, and the road-not-taken argument stays in the pull request description where it belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Rewrite the uint8 tier tests on the suite's existing conventions The two tier-aware tests carried their own machinery: a UInt8TierFuncs struct, an AvailableUInt8Tiers() helper building a vector of tiers, plus RecordProperty, stdout tier reporting, a VECSIM_REQUIRE_UINT8_TIER environment gate and a GTEST_SKIP. Nothing else in test_spaces.cpp works that way, and the file already had a pattern for both jobs. The boundary test now uses the step-down ladder from UINT8L2SqrTest: read the CPU features, then walk SVE2, SVE, NEON_DOTPROD, NEON and AVX512, asserting at dimension 33,025 that each chooser hands back that tier's Choose_UINT8_*_implementation for all three metrics, clearing the tier's flags before the next block and ending on the scalar kernel once none are left. That replaces an EXPECT_NE gated on "does this host have any tier" and says more: every tier the host has is still served at the bound, not merely something other than the scalar kernel. The above-bound assertions now pass the real feature struct rather than nullptr, so the guard is shown overriding the host's best tier. The per-tier exactness test now mirrors UINT8_full_range_test: call each tier's chooser directly under the same ifdef and feature guard, three ASSERT_EQ against the scalar baselines, same message style. Norms come from test_utils::integral_compute_norm instead of a hand-computed sqrt, which also exercises the widened norm accumulator, and each dimension gets its own vectors so there is no saving and restoring of the four norm bytes. Each metric now runs on the pair that maximises its total, L2 on all-255 against all-0 and inner product on all-255 against itself, so both reach 65,025 * dim, which is 2,147,450,625 at the bound and the largest total a signed 32-bit accumulator holds. The previous ramp-against-ones pair peaked near 1.07e9, half the limit, so it never stressed the bound it was placed to validate. Cosine keeps the ramp because the all-zero vector has no norm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Cover the three SQ8 dispatcher guards, which no test reached codecov/patch failed on this PR and it was right. The SQ8_SQ8 guards were the only new lines with no test touching them: the suite does call IP_SQ8_SQ8_GetDistFunc and L2_SQ8_SQ8_GetDistFunc, but only at small dimensions, so the `return ret_dist_func` inside each guard was never taken. That matters more than the coverage number. Those three guards are the whole reason the SQ8 kernels are safe under this cap: they call the shared UINT8_InnerProductImp directly and never pass through a uint8 chooser, so without the guard they are the one path left able to wrap a signed 32-bit total. Shipping them untested would have meant shipping the SQ8 protection on the strength of an argument rather than a test. Three assertions in the boundary test, alongside the uint8 ones, asserting each SQ8 dispatcher hands back its scalar kernel by name above the bound. 1024/1024 on Xeon Platinum 8375C. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fold the uint8 cap tests into the suite's own fixtures The previous round still read as foreign code. Three bespoke TEST_F bodies with sentence-length names, hand-rolled dimension loops, per-metric pointer aliases, static_asserts inside a test body, relative-error comparisons and paragraphs of prose, none of which appear anywhere else in test_spaces.cpp. The file already had a pattern for each job. Dimension sweeps are a second INSTANTIATE_TEST_SUITE_P over an existing fixture, exactly as SQ8_FP16_SIMD_HighDim extends SQ8_FP16_SpacesOptimizationTest. Adversarial values across every tier are a TEST_P in the optimization fixture calling each Choose_UINT8_*_implementation directly, exactly as UINT8_full_range_test does. So: UINT8OptFuncsNearCap instantiates UINT8SpacesOptimizationTest at 32,960 / 32,993 / 33,023 / 33,024 / 33,025, which gets the existing L2, IP and cosine step-down ladders running at near-cap dimensions for free, including the assertion that each tier is still handed out at the cap. That is what the old test's EXPECT_NE and its "does this host have a tier" gate were reaching for, and the fixture already does it properly. UINT8_max_value_test is the worst-case counterpart to UINT8_full_range_test: all-255 against all-0 for L2 and all-255 against itself for IP and cosine, both totalling 255 * 255 * dim, the largest total the 32-bit accumulators hold at the cap. It also adds the NEON and NEON_DOTPROD blocks that UINT8_full_range_test never had. UINT8_DispatcherCapFallback keeps only what is dimension-specific: the cap pinned arithmetically, all six uint8 and SQ8_SQ8 choosers returning their scalar kernel above it, and the scalar kernels being exact there. Exact integer comparisons throughout, no tolerances. 132 added lines where the previous round had 247, and no new test-only machinery. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Compare uint8 IP within a ULP, which is all the two kernel forms agree to Bugbot caught a real defect in the previous commit. UINT8_max_value_test asserted exact float equality between the scalar inner product and every tier's, and the two are not the same expression. The scalar kernel and the AVX512 kernel compute `1 - sum` in integers and convert once; NEON and SVE compute `1.0f - float(sum)`. Those round differently once the total passes 2^24. Measured rather than argued, by evaluating both forms directly: with all-255 bytes, 839 of the 32,994 dimensions in [32, 33025] disagree, and dimension 32,960, which UINT8OptFuncsNearCap instantiates, is one of them: -2143223936 against -2143224064, exactly 128 apart, one float ULP at that magnitude. So the previous commit would have failed on ARM while passing on x86, which is also why the Xeon run did not catch it. IP is now compared within one ULP, computed from the baseline rather than hardcoded. L2 and cosine stay exact: both convert the total to float and do no further integer arithmetic, so every tier and the scalar path produce bit identical results there. The existing suites that UINT8OptFuncsNearCap newly instantiates are safe as they stand: their populate_uint8_vec data at all five dimensions produces totals where both forms agree, checked the same way. That ARM and x86 differ by a ULP on the same uint8 inner product is pre-existing on main, not introduced here, and is a separate question from the cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Give the int8 NEON inner product helper internal linkage too Defect 2 in this PR is UINT8_InnerProductImp, defined identically in IP_NEON_UINT8.h and IP_NEON_DOTPROD_UINT8.h and merged by link order. INT8_InnerProductImp is the same defect one datatype over, in the same two directories, and the fix for uint8 left it untouched. Measured on Graviton4 with gcc 12 against this branch before the change: shared weak symbols, NEON.o vs NEON_DOTPROD.o: 64 of which INT8_InnerProductImp: 64 of which UINT8_InnerProductImp: 0 Every remaining collision was the int8 twin, which is both evidence the uint8 fix works and evidence it did half the job. The consequence is the same. IP_space.cpp selects Choose_INT8_IP_implementation_NEON on features.asimd alone, with no dotprod requirement, and that wrapper calls INT8_InnerProductImp. If the DOTPROD-compiled body is the one the linker keeps, plain NEON executes sdot, which needs asimddp: SIGILL on an armv8-a core without it. Nothing in the source decides which body survives. Two definitions of this same name already avoid the collision, which is the in-repo precedent: IP_AVX512F_BW_VL_VNNI_INT8.h declares it static inline, and IP_SVE_INT8.h has a different template parameter list. The helper is referenced only inside its own headers, so static is sufficient and local. After, on the same host: 0 shared weak symbols, NEON.o has zero sdot and keeps its 472 smull, NEON_DOTPROD.o keeps all 428 sdot. 858/858 tests pass. Not included here: a nm zero-collision CI gate would fail immediately on the 168 SVE/SVE2 collisions this PR does not fix, so it belongs with MOD-17759 where those are tracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use uint64_t for integral norm accumulation * Give the SVE int8 helper internal linkage too, completing the helper sweep The previous commit made the two NEON int8 definitions static and missed this one. IP_SVE_INT8.h is compiled into both SVE.cpp (-march=armv8-a+sve) and SVE2.cpp (-march=armv9-a+sve2), so INT8_InnerProductImp carried one mangled name with two independently generated bodies, 8 instantiations of exactly the defect the previous commit claimed to fix. Measured on Graviton4, shared weak symbols between SVE.cpp.o and SVE2.cpp.o: before: 160, including 8 INT8_InnerProductImp after: 152, zero InnerProductImp of either datatype 858/858 tests pass. What remains between those two objects is the wrappers, 8 instantiations each of UINT8_InnerProductSIMD_SVE, UINT8_CosineSIMD_SVE, UINT8_L2SqrSIMD_SVE and their int8 counterparts. Making a helper static removes the helper's collision but leaves its callers colliding, and those callers now differ more than before since each refers to its own unit's helper. That residue is latent rather than live: across all shared SVE/SVE2 symbols both compilations use identical instruction sets, 57 distinct opcodes each with no difference in either direction, so no SVE2-only instruction can reach an SVE-only core. Closing it means giving the wrappers internal linkage across IP, L2 and Cosine for every data type, which is MOD-17759. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trim the linkage comments to one line each, and drop the ticket and reviewer name The static changes carried up to three lines of explanation each, wedged between the template header and the signature, restating what the commit message and the pull request already cover. One line above the template is enough to stop someone deleting the keyword as redundant; the reasoning belongs in the history. Twelve comment lines become six. Also removed the ticket number and reviewer handle from the norm test's comment, per review. The defect and why it went unnoticed stand on their own; who reported it and under which ticket is what the pull request and git history are for, and a name in a comment goes stale the moment the file outlives the review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Describe the changes in the pull request
The issue
SQ8 storage metadata held
sum = Σx[i]andsum_squares = Σx[i]²over the input values. Every kernel is written over the reconstructionx_r[i] = min + delta * a[i], which is what a quantized blob actually holds. The two differ by the quantization error, about 0.4% of||x||². That is larger than the distance between two similar vectors, so L2 came back wrong and often negative.Measured at dimension 128 on two near-duplicate vectors:
The fix
Make the metadata describe
x_r. Nothing else changes: no kernel is touched, and the blob keeps the same layout, slot count and FP32 slot types.The existing algebra is already exact once the sums describe
x_r:So
quantize()counts the quantized bytes as exact integers, then derives both sums from them in double and stores FP32 as before:sum = dim*min + delta*q_sum; sum_squares = dim*min*min + 2.0*min*delta*q_sum + delta*delta*q_sum_squares;q_sumandq_sum_squaresare local counters, not stored fields.q_sum_squaresisuint64_t: each byte contributes up to255², so a 32-bit counter passesUINT32_MAXjust above dimension 66051, and a wrapped counter would corrupt the stored norm rather than round it.SUMhas exactly one reader, the inner product algebra above, which is what makes a metadata-only fix sufficient.What this does not fix
L2 is now correct, but not provably non-negative. Residual FP32 cancellation in
sum_sq_x + sum_sq_y - 2*IPis about1e-7 * ||x||², and two identical blobs have a true distance of zero, so self-distance lands just below it (measured-4.9e-4at dimension 512). The withdrawn revision below did guarantee non-negativity; this one does not. Tests bound the result by that noise floor rather than by zero, and the gap is recorded for follow-up.Storage and query describe different quantities
A query is never quantized, so its metadata keeps sums over the query itself: the asymmetric distance is
Σx_r² + Σy² - 2Σ(x_r * y). Three test assertions compared the query sum against the storage sum. They only ever agreed because both used to be the input sum, which is the same confusion this change fixes in the product code.Tests
SQ8_SQ8_L2_is_non_negative_and_matches_reconstructionandSQ8_FP32_L2_matches_reconstruction_against_float_query: near-duplicate vectors from dimension 4 to 512, scalar and dispatched kernels, against a double reference overx_r. Near-duplicates are the case that exposes this, because the true distance is small enough for the 0.4% mismatch to dominate.The asymmetric test is new, and its absence is why the defect survived: the existing asymmetric L2 tests run dimensions 1, 5, 7 and 15 against an absolute tolerance of
0.01, and 0.4% of||x||²at dimension 5 is about0.007, which fits underneath.Tolerance is
8 * FLT_EPSILON * (norm_x + norm_y), scaled to the norms because the error floor is FP32 cancellation, which does not scale with the distance. A flat bound is thousands of times the distance it guards at small dimensions and would let a kernel returning zero pass.SQ8_SQ8_L2_self_distance_is_near_zero: near zero, not exactly zero, for the reason above.Considered and rejected
An earlier revision made the L2 metadata integer (
uint32Q_SUM/Q_SUM_SQUARES), added areconstructed_l2_sqrhelper regrouping the quadratic term soS1 + S2 - 2Qstays an exact integer, and rewrote 18 kernel files to combine in double. That buys exactly-zero self-distance and protection against FP32 cancellation when two vectors share a large offset, at 24 files, +630/-277, per-metric slot semantics, and 1 to 4.8 ns per distance call.Those properties address the large-shared-offset case, not the defect above. This revision fixes the defect at zero per-call cost without touching a kernel. Carried forward: FP32 cancellation under a large shared offset, non-negative and exactly-zero self-distance, and the
MAX_EXACT_DIMcomment describing a bound the code does not have.Verification
CI green at
d5b467eb:basic tests,sanitizer / Test ubuntu-latest sanitizer(ASan, both suites),codeql-analysis / Analyze (cpp),coverage / codecov job,codecov/patch,codecov/project,spellcheck, Cursor Bugbot. Locally:check-format.Which issues this PR fixes
Main objects this PR modified
src/VecSim/spaces/computer/preprocessors.h: the derivation and the metadata documentation.tests/unit/unit_test_utils.h,tests/utils/tests_utils.h: the two test mirrors of the quantizer.tests/unit/test_spaces.cpp,tests/unit/test_components.cpp.Mark if applicable
Note
Medium Risk
Changes core SQ8 quantization metadata used on every indexed vector and L2 search path, but scope is limited to preprocessor/test quantizers with no kernel or layout change; incorrect sums would break distance ordering.
Overview
Fixes wrong and often negative SQ8 L2 distances by making storage metadata match what the distance kernels assume:
sumandsum_squaresare over the reconstructed vectorx_r[i] = min + delta * a[i], not the pre-quantization input. The mismatch was on the order of ~0.4% of||x||², which could dominate true distance between similar vectors.QuantPreprocessor::quantize()now accumulates quantized byte sums (withuint64_tfor squared-byte totals on L2), then derivessumandsum_squaresin double and stores FP32 metadata as before. Blob layout and distance kernels are unchanged.Tests and test quantizer helpers (
ComputeSQ8Quantization,quantize_float_vec_to_sq8_with_metadata) follow the same rules. FP16 preprocessor tests assert query metadata against input sums, not storage reconstruction sums. Newtest_spacescases check SQ8↔SQ8 and SQ8↔FP32 L2 against a double reference on near-duplicate vectors, with tolerances tied to FP32 cancellation noise rather than a loose absolute bound.Reviewed by Cursor Bugbot for commit e96910f. Bugbot is set up for automated code reviews on this repo. Configure here.