Repository navigation
[muilt-vetor] Add standalone MinMax8 x MinMax4 MaxSim kernel - #1494
juchen-ms (partychen) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The new unsafe SIMD paths require cross-platform validation and final human review.
0 open findings
What changed in this PR
Adds a standalone quantized MaxSim kernel to DiskANN’s matrix kernels, leaving distance API integration for a follow-up.
Changes:
- Implements packed-A, B-tiled computation for Scalar, V3, V4, and Neon.
- Adds compensated inner products and NaN-aware maximum reduction.
- Adds supporting SIMD conversions and regression coverage.
| File | Description |
|---|---|
| diskann-wide/src/reference.rs | Adds reference u32-to-f32 conversion. |
| diskann-wide/src/emulated.rs | Adds and tests 16-lane i32-to-f32 conversion. |
| diskann-wide/src/arch/x86_64/v4/conversion.rs | Adds and tests AVX-512 conversion. |
| diskann-wide/src/arch/x86_64/v3/conversion.rs | Adds and tests split-vector conversion. |
| diskann-wide/src/arch/aarch64/u32x4_.rs | Adds and tests Neon conversion and reinterpretation. |
| diskann-quantization/src/matrix_kernels/maxsim/packed_minmax8_x_unpacked_minmax4.rs | Implements the kernel, backends, and regression tests. |
| diskann-quantization/src/matrix_kernels/maxsim/mod.rs | Registers the kernel pending integration. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1494 +/- ##
==========================================
+ Coverage 90.94% 91.07% +0.12%
==========================================
Files 586 587 +1
Lines 118391 121439 +3048
==========================================
+ Hits 107675 110603 +2928
- Misses 10716 10836 +120
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Support all 1-8 bit A/B combinations through shared packing, tiled execution, compensation and maximum-inner-product reduction. Retain the tuned byte backends alongside full-range i16 paths, keep quantization widths out of the numeric microkernels, and add the required SIMD conversions and correctness coverage. Leave factory, fallback and public distance integration to follow-up PRs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74814a43-c576-493a-8783-585ef48c3f1c
f074880 to
e465898
Compare
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
Thanks Junkui. This is a simplification over the earlier PR! There still is some coupling between the lower-level kernels and some of the higher data preparation stuff.
A few other things that stand out to me:
- It makes heavy use of post-monomorphization errors via
const { assert!(...) }. These aren't necessarily wrong, but they are kind of hard to debug when they trigger, don't compose with higher level code well, and to me often indicate that there is another way of structuring the code so they aren't needed. I'm not saying get rid of them, but that over reliance could indicate a missing abstraction. - Why are 16-wide accumulators used for all architectures? This seems like it could be problematic for Neon or Scalar. Have you checked that the generated code isn't spilling too much and is reasonable?
I know a few other folks are planning on reviewing this as well - let's see what they have to say.
e360602 to
2538cc7
Compare
Thanks for the feedback. I’ve moved SIMD-specific operations behind I’ve also added MR=8 alternatives for Neon, V3, and Scalar’s i16 path rather than requiring 16-wide accumulators everywhere. Earlier V3 assembly inspection showed register-resident accumulators in the full contraction loop, but stack traffic around compensation and tails; I’m not claiming all configurations are spill-free.
|
Move bulk decoding into BitSlice and zero-pad prepared B rows for a unified pack loop. Decouple backend representations behind fixed-size operations, retain smaller row-panel choices, and replace redundant test sweeps with directed coverage. Make B preparation independent of backend and row-panel types by accepting a code converter. Keep backend capability validation at the Driver construction boundary. Give each backend the full microkernel compensation and reduction stage through max_into with borrowed coefficients. Keep MR16 contraction intact while V3 reduces native eight-row halves. Define backend operations explicitly and share the compensation formula; preserve floating-point order and NaN semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2538cc7 to
41710a5
Compare
No new dependencies. Public additions are backward-compatible: dense
BitSlicebulk decoding and supporting SIMD trait implementations. Existing distance APIs and canonical MinMax storage remain unchanged.Reference Issues/PRs
Reworks the matrix-kernel portion of #1394 into a standalone implementation. Uses the SIMD reinterpretation and
maddubssupport from #1445 and #1479.What does this implement/fix? Briefly explain your changes.
Adds an internal MaxSim matrix kernel for MinMax A/B inputs with 1-8 bits per code, supporting all 64 bit-width combinations through the available numeric layouts.
Execution
PackedAowns the prepacked query codes and compensation coefficients and can be reused across B comparisons.Drive -> PanelKernel -> MicroKernelstructure decodes each B tile once and reuses it across A panels. Compensation precedes max reduction; no full pairwise score matrix is materialized.BitSlice::decode_into, rather than MinMax-local 4/8-bit decoding branches. Output length is validated before writing or invoking the conversion callback.validargument or separate partial-K contraction.ExtraWideexposes backend operations for loads, accumulation, score storage, and compensated reduction. Concrete array conversions replace the generic microkernel's lane-count const assertions. Construction-time checks for legal NR/backend bit widths remain.Available layouts
All layouts support A_BITS=1-8. MR counts A rows per microkernel; NR is a separate parameter in 1-8. These are supported layouts, not a universal optimal configuration or automatic selection policy.
u8/i8i16/i16u8/i8i16/i16u8/i8i16/i16u8/u8The byte limits prevent signed-byte misinterpretation and, on V3, saturation of the intermediate i16 sum. Full-range eight-bit B uses the i16 path on Scalar/x86 and unsigned byte dots on Neon.
Behavior
Each drive overwrites its output with unnegated maximum inner products. NaNs are ignored; empty B or all-NaN candidates leave negative infinity. The internal driver requires nonempty A, positive K, and matching dimensions. K is bounded by
i32::MAX / ((2^A_BITS - 1) * (2^B_BITS - 1)); conversion to f32 may still round.Tests separately cover decoding, packing, arithmetic limits, supported bit widths, partial panels, tile reuse, smaller MR layouts, compensation, and NaN reduction. Repeated whole-kernel combinations have been reduced in favor of focused cases.
Any other comments?
Scope: Factory/ISA selection policy, public prepared-query distance integration, FFI/externally owned query views, and benchmark integration remain follow-up work.
PackedAstayspub(crate).Performance: MR=16 is retained alongside narrower layouts, not claimed optimal for every architecture or query size. Earlier exploratory timings and assembly observations are not validation of this head; current-head native x86 performance and spill analysis still need confirmation.
Validation at
2538cc7-- Windows ARM64 host:cargo fmt --all --checkcargo test -p diskann-quantization --release --lib -- bits::slice::tests matrix_kernels::maxsim::packed_minmax_x_unpacked_minmax --test-threads=1cargo clippy -p diskann-quantization -p diskann-wide --all-targets -- -D warningscargo clippy -p diskann-quantization -p diskann-wide --all-targets --target x86_64-pc-windows-msvc -- -D warningsNative V3/V4 execution, SDE/Miri, and coverage were not rerun as part of these checks. Cross-compilation is not runtime validation.