Repository navigation
[multi vector] Add tiled MinMax MaxSim kernels - #1394
juchen-ms (partychen) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The public factory has a Reference ISA compatibility issue, and V3/V4 runtime paths remain unvalidated on x86-64.
Pull request overview
Adds prepared, tiled MaxSim kernels for MinMax8 queries against MinMax4 documents with ISA-specific dispatch.
Changes:
- Adds public prepared-kernel and factory APIs.
- Implements Scalar, AVX2/V3, AVX-512/V4, and ARM64 Neon kernels.
- Adds packing, fused compensation/reduction, validation, and reuse tests.
File summaries
| File | Summary | Final review note |
|---|---|---|
diskann-quantization/src/multi_vector/mod.rs |
Re-exports the factory. | — |
diskann-quantization/src/minmax/multi/mod.rs |
Registers and exports kernel modules. | — |
diskann-quantization/src/minmax/multi/kernel.rs |
Defines kernel interfaces. | — |
diskann-quantization/src/minmax/multi/factory.rs |
Provides ISA dispatch and prepared-kernel construction. | Moderate: MaxSimIsa::Reference is reported available but rejected by this factory. |
diskann-quantization/src/minmax/mod.rs |
Exposes MinMax APIs. | — |
diskann-quantization/src/matrix_kernels/maxsim/mod.rs |
Registers the MinMax matrix kernel. | — |
diskann-quantization/src/matrix_kernels/maxsim/minmax8_x_minmax4.rs |
Implements packing, tiling, compensation, SIMD kernels, and tests. | Nit: V3/V4 paths lack x86-64 runtime coverage. |
Review details
Suppressed comments (2)
diskann-quantization/src/matrix_kernels/maxsim/minmax8_x_minmax4.rs:785
- The V3/V4 implementations here are only compile-checked in the stated ARM64 validation: the ISA tests return early when the requested x86 ISA is unavailable, so the
pdep/VNNI path and its lane reduction are not exercised. Please add or run an x86-64 runtime test (or an equivalent testable emulation) covering these kernels before relying on this new code.
micro_kernel!(V3, 16, micro_kernel, {8, 7, 6, 5, 4, 3, 2, 1});
micro_kernel!(V4, 16, micro_kernel, {8, 7, 6, 5, 4, 3, 2, 1});
diskann-quantization/src/minmax/multi/factory.rs:171
MaxSimIsa::Referenceis documented as an always-available selector (is_available()returns true), and the existing MinMax implementation provides the reference MaxSim path, but this new public factory rejects it unconditionally. Callers that useis_available()to preflight an ISA therefore receiveNotSupportedfor a value reported as buildable. Please add a reference adapter (or make support for this factory explicit in the ISA API/documentation).
MaxSimIsa::Reference => Err(NotSupported {
isa,
reason: "reference kernel unavailable",
}),
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 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 #1394 +/- ##
==========================================
+ Coverage 91.98% 92.09% +0.11%
==========================================
Files 586 593 +7
Lines 118318 119897 +1579
==========================================
+ Hits 108831 110420 +1589
+ Misses 9487 9477 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
There's a lot going on here. It does add a minmax8x4 kernel, but basically builds a second matrix-kernel decomposition architecture rather than extending the existing abstractions introduced in 1368.
The point of the machinery introduced there was to use enable better higher-level experimentation of cache tilings and ordering without fully relying on Miri or checked accesses for correctness debugging. All of these pieces could then be tested, reviewed, and reused independently.
Here, packing and traversal are all reimplemented manually with inconsistent bounds tracking applied. This is hard to test, validate, or reuse.
My suggestion would be to follow the design philosophy of 1368 and build the traversal mechanism out of lower-level parts (which we can then reuse for other quantization kernels). In addition, matrix kernels should not own any non-scratch state because that also makes them much more difficult to reuse.
I also have some other design concerns:
- The microkernel is currently doing a lot, and I feel that there has to be a more efficient unpacking than doing this much bit-twiddling in general purpose registers. One design worth considering incrementally unpacking B and reusing that for the whole A traversal (flipping the cache tiling order - which again is easier to do via paneled views). This can perhaps be coupled with a different permutation strategy of the A-side query for more efficiency.
ExtraWideshould as much as possible keep the associated types opaque to the caller (likeExtraWidein the f32 kernel). This makes it significantly easier to express architecture specific optimizations of coarser grained kernels without the profoundly heaverSIMDVectorand such constraints.- How many of the Miri exceptions are really needed? Miri can emulate a good number of Neon and AVX2 intrinsics.
- Directed testing of the various steps is pretty sparse. Especially given its ad-hoc mix of checked, unchecked, and
Bounds based indexing. UsingBounds based checking in a more disciplined way at least provides higher confidence of integration-based tests.
Have you considered making the A-side packing even more aggressive, splitting into groups of even and odd indices? The idea there is that it would naturally fit the order of unpacked nibbles much better and avoid a lot of the interleaving logic. Basically, a lot the micro-kernels are concerned about restoring a dimension ordering that we anyways control.
Mark Hildebrand (@hildebrandmw) Thanks for the detailed review. I reworked the implementation around the abstractions from #1368 in
I also added/ran directed packing, tail, bounds, driver/register, factory-equivalence, repeated-computation, empty-input, and NaN-compensation coverage. Native x86-64 V3/V4 tests, ARM64 cross-Clippy, and the V4 Miri test pass. For the 1,000-document 16x16 workload on a Xeon Platinum 8370C, the latest release measurements are:
The 250-dimensional Existing path has a scalar remainder, so I am keeping the 250- and 256-dimensional results explicitly separate. |
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
I got through packed.rs. I think there's a little we can do to make the group access a bit semantically tighter. I'm still working my way through the rest. Is there a concern about using the same blocking strategy across all architectures?
Thanks! I addressed the
For V3, yes. I measured register and cache blocking on a Xeon Platinum 8370C (AVX-512) with release builds, across the multi-vector shapes from 16x16x256 to 64x1250x512. I didn't use
|
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
I have largely structural comments for this round. There is a lot of cross-talk between components. For example, EvenOddLayout64 duplicates dimensions that should already exist in a large number of other places, is AVX2 specific, and doesn't layer well with views. Lots of panic locations. The Decoder trait is hard-coupled with the EvenOdd64Layout, and ExtraWide is doing the entire micro-kernel, etc.
This feels like it's headed in the right direct, but still needs tightening of its underlying abstractions (and maybe bringing in new ones when current ones aren't doing the trick).
One big request I have is whenever there is some missing functionality in diskann-wide, please add it (or ask me to) so we can improve that library for everyone.
| } | ||
|
|
||
| impl EvenOdd64Layout { | ||
| pub(super) const BLOCK: usize = 64; |
There was a problem hiding this comment.
Is 64 a universally good number? It means we can't use AVX-512 to do the on-the-fly unpacking and makes Arm work a little harder.
There was a problem hiding this comment.
Agreed—64 is not universally optimal. It originally matched a 32-byte AVX2 load of packed 4-bit values, producing two 32-byte channels for the even/odd layout.
I compared 32-, 64-, and 128-dimensional blocks on a Xeon Platinum 8370C, keeping V4’s MR16/NR8/PACK8 unchanged. The 128-dimensional variant used native 64-byte AVX-512 decoding, rather than just increasing the padding. Across 22 multi-vector shapes, with three repeated runs, 32-dimensional blocks reduced geometric-mean latency by about 1.7% relative to 64, while 128-dimensional blocks increased it by about 5.8%. Smaller blocks helped some tail dimensions; larger blocks introduced more padded computation.
For now, I’d keep 64 as the shared prepared-data format: the measured benefit does not yet justify introducing architecture-specific layouts. This is a tradeoff supported by this x86 workload, not a claim that 64 is best everywhere. I haven’t benchmarked this on native ARM hardware, so your concern about Neon remains open.
…1445) - [x] Does this PR have a descriptive title that could go in our release notes? - [ ] Does this PR add any new dependencies? No. - [x] Does this PR modify any existing APIs? Adds trait implementations only. - [x] Is the change to the API backwards compatible? - [ ] Should this result in any changes to our documentation? No separate documentation changes are needed. #### Reference Issues/PRs Extracted from #1394 to land the reusable diskann-wide support independently. #### What does this implement/fix? Briefly explain your changes. Adds bit-preserving SIMD reinterpretations from word lanes to byte lanes: - V3: `u32x8` → `u8x32` and `i8x32`. - V4: `u64x8` → `u8x64` and `i8x64`. - Matching little-endian `Emulated` implementations for all four conversions. Tests follow the existing conversion-test macro and architecture-check conventions. They compare raw bytes using distinct byte positions, signed-byte boundary patterns, and the existing seeded test driver. The change is limited to diskann-wide. No MaxSim consumers, custom intrinsics, or unrelated refactors are included. #### Any other comments? Local validation passed: - `cargo test -p diskann-wide` - `cargo fmt --all` - `cargo clippy --workspace --all-targets -- -D warnings` - `cargo +nightly miri test -p diskann-wide --lib reinterpret_` --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
33ce33c to
25808c6
Compare
Prepare MinMax queries and synchronized panels using the existing matrix-kernel drivers. Add Scalar, V3, V4, and Neon execution paths and reuse the V3 maddubs wrapper. Integrate MinMax8/MinMax4 with the shared multi-vector benchmark, including input fixtures, checked allocation, regression checks, and CLI coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
25808c6 to
a1db87c
Compare
Separate layout and operand preparation, validate canonical extents in MinMaxMeta, and use shared span interfaces with backend-specific panel registration. Keep SIMD queries fixed-size and move prepared byte accounting into operand helpers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
No new dependencies are added. Existing API signatures and valid-input behavior are preserved. Invalid
MinMaxMetadimensions or byte extents are now rejected during construction; the canonical data format is unchanged.Reference Issues/PRs
See also #1368 for the existing matrix-kernel decomposition and #1479 for the upstream
diskann-wideinstruction wrappers used by this implementation.What does this implement/fix? Briefly explain your changes.
Adds prepared, tiled MaxSim kernels for MinMax8 queries against canonical MinMax4 documents, with Scalar, x86-64 V3/V4, and ARM64 Neon implementations.
Query values and compensation metadata are prepared once. Each computation borrows a document's quantized vectors and overwrites one compensated MaxSim distance per query vector, without materializing a full pairwise score matrix.
API
The factory supports automatic ISA selection and explicit Scalar, V3, V4, and Neon requests. Unavailable architectures, unsupported prepared extents, and dimensions exceeding the integer-accumulator limit of 1,122,867 return
NotSupported.MaxSimIsa::Referenceis not supported by this factory.Layout and execution
The implementation uses the existing
Drive,PanelKernel, andMicroKernelmachinery, shared packed/unpacked views, andSlice/Elementsinterfaces.layout.rsdefines the shared MinMax dimension ordering and padding contract.a.rsprovides borrowed packed-query views with synchronized compensation metadata.b.rshandles canonical document-row access, scratch preparation, nibble unpacking, and prepared B views.minmax/multi/query.rs, above the matrix-kernel layer.Within each 64-dimensional block, even coordinates precede odd coordinates:
A uses
BlockTransposed<u8, MR, PACK>storage. B is unpacked into contiguous byte-valued rows with the same dimension order. Missing dimensions and unused query rows are zero-padded; compensation uses the original dimension.B-first traversal prepares each document tile once and reuses it across all A panels:
The driver obtains prepared working-set sizes from
AView::panel_bytes()andScratch::row_bytes(). These include padding and compensation metadata; B's size reflects the unpacked representation, not its canonical packed row size.Two per-call scratch buffers hold B values and metadata and are reused between tiles. Prepared query state remains read-only, and documents are not modified.
Architecture configurations
maddubsfollowed bymaddSupported panel geometries and remainder instances are registered per backend. SIMD query representations are fixed-size register arrays: loading and dot accumulation always process the complete prepared patch, without
Optionor active-row-dependent half-panel selection.The nibble-unpacking interface accepts arbitrary-length slices independently of SIMD register width. Backend implementations handle complete chunks and bounded tails, including the unused high nibble of odd-dimensional documents.
Validation and behavior
MinMaxMeta::try_new()validates the canonical dimension, row stride, total byte extent, and allocation limits. The validated row stride is cached. ExistingMinMaxMeta::new()delegates to this validation and retains a documented panicking convenience API. Benchmark fixtures use the shared fallible constructor instead of maintaining separate size checks.The computation:
f32::MAXfor empty documents and zero for nonempty zero-dimensional documents.Directed tests cover query packing, padding, nibble unpacking, logical group bounds, metadata alignment, prepared byte footprints, cache-tile selection, scratch reuse, unsigned accumulator conversion, and equivalence with the pairwise implementation.
Benchmark integration
Extends the existing
multi-vectorbenchmark feature withelement_type: "minmax8", quantized fixtures, regression support, example jobs, and ISA-specific performance inputs.Quantization and query preparation are outside the timed region. The benchmark includes per-call scratch work and reports
ns/IPwhile repeatedly scoring the same document matrix.Any other comments?
This revision retains the shared 64-dimensional MinMax prepared format. It does not introduce a generalized column-order policy or adapter, a new
Patchabstraction, or changes to sharedBlockTransposedsemantics.Fixed-size query processing deliberately favors a regular micro-kernel. It can increase work for partially filled query panels and is not presented as a universal performance improvement. The choice of a shared 64-dimensional format is also a workload-dependent tradeoff, not a claim of optimality on every architecture. No native ARM performance measurements are claimed.
Local validation completed for this revision
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo test -p diskann-quantization --lib minmax --quietcargo test -p diskann-benchmark --features multi-vector multi_vector --quietcargo test -p diskann-benchmark --no-default-features multi_vector --quietcargo clippy -p diskann-quantization --lib --tests --target aarch64-pc-windows-msvc -- -D warningscargo +nightly miri test -p diskann-quantization --lib bytes_includeThe listed Miri run used
RUSTFLAGS=-Ctarget-cpu=x86-64-v3andMIRIFLAGS=-Zmiri-disable-isolation -Zmiri-strict-provenance. These are focused checks, not a full-workspace test or Miri run.