Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Add the missing conversion coverage and correct the Neon documentation classifications.
Review effort: Lite
Findings: None
What changed in this PR
Adds AArch64 NEON full-precision kernels for 1-, 2-, and 4-bit quantized inner products, with updated spherical dispatch.
Changes:
- Added NEON widening and integer-to-float conversions.
- Implemented quantized NEON kernels.
- Updated spherical dispatch and AArch64 code generation.
| File | Summary |
|---|---|
diskann-wide/src/arch/aarch64/u32x4_.rs |
Added widening conversion support. |
diskann-wide/src/arch/aarch64/f32x4_.rs |
Added integer-to-float conversions. Moderate (1 vote): add tests for u32x4 → f32x4 and u16x4 conversion paths. |
diskann-wide/src/arch/aarch64/double.rs |
Added wider NEON conversion support and tests. |
diskann-quantization/src/spherical/iface.rs |
Updated direct NEON dispatch. |
diskann-quantization/src/spherical/__codegen/aarch64.rs |
Added NEON kernel instantiations. |
diskann-quantization/src/bits/distances.rs |
Added 1-, 2-, and 4-bit NEON kernels. Nit (1 vote): correct the Neon table classifications. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9efd746 to
a90e92b
Compare
|
Appreciate the comments Mark, I've done the following:
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1431 +/- ##
==========================================
- Coverage 91.51% 90.58% -0.94%
==========================================
Files 527 567 +40
Lines 103055 112347 +9292
==========================================
+ Hits 94309 101767 +7458
- Misses 8746 10580 +1834
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
These changes tested on wiki-1M with diskann-benchmark (exhaustive-minmax-quantization) using 8 threads C100 VM
Later I will test out the prefetching changes #1408 on top as well, measure cache miss rates. |
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
Thanks! Some housekeeping requests for diskann-wide and a quick question on the 1-bit kernels.
|
PR is now ready for more review Mark Hildebrand (@hildebrandmw)
Thanks! |
78f3f7e to
bcf61ef
Compare
|
Ready for review again, no new changes since my last comment. Just fixed merge conflicts |
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
Thanks - a few small comments on my end. The biggest request is on the 1-bit kernel. LLVM appears to optimize sequences based on SIMDSelect, and & the same as when vtstq is used explicitly, so I'd prefer to keep the kernel in terms of more general machinery.
| /// | ||
| /// See: [`vtstq_u32`] | ||
| #[inline(always)] | ||
| pub fn vtstq_u32(self, x: uint32x4_t, y: uint32x4_t) -> uint32x4_t { |
There was a problem hiding this comment.
For this PR - I think we can get away without introducing an intrinsic because LLVM kinda already knows how to handle these cases.
By this, I mean that the code generation between foo and bar in the linked compiler explorer pane is exactly the same, despite one using vtstq_u32 and the other using a more circuitous route using vandq, vceqq and vbslq. (Note that the LLVM version used by currently doesn't want to emit cmtst anyways, probably due to this issue)
The point is, we can get the same code generation using a simpler
let zero = f32x4::splat(arch, 0.0);
// This sequence below is effectively calls
// `bar` from the linked compiler explorer.
let mask0: u32x4 = y_word & bits0;
s0 = s0 + mask0.select(x0, zero); and save a lot of bespoke machinery.
| test_utils::ops::test_lossless_convert!(u8x32 => i16x32, 0x84c1c6f05b169a20, test_neon()); | ||
| test_utils::ops::test_lossless_convert!(i8x16 => i16x16, 0x84c1c6f05b169a20, test_neon()); | ||
| test_utils::ops::test_lossless_convert!(i8x32 => i16x32, 0x84c1c6f05b169a20, test_neon()); | ||
| test_utils::ops::test_lossless_convert!(u16x8 => f32x8, 0x84c1c6f05b169a20, test_neon()); |
There was a problem hiding this comment.
Can we add u8x8 => f32x8 and u8x16 => f32x16 as well for completeness?
| test_utils::ops::test_lossless_convert!(u8x32 => i16x32, 0x84c1c6f05b169a20, test_neon()); | ||
| test_utils::ops::test_lossless_convert!(i8x16 => i16x16, 0x84c1c6f05b169a20, test_neon()); | ||
| test_utils::ops::test_lossless_convert!(i8x32 => i16x32, 0x84c1c6f05b169a20, test_neon()); | ||
| test_utils::ops::test_lossless_convert!(u16x8 => f32x8, 0x84c1c6f05b169a20, test_neon()); |
There was a problem hiding this comment.
Can we add u8x8 => f32x8 and u8x16 => f32x16 as well for completeness?
| test_utils::ops::test_sumtree!(f32x4, 0x828bd890a470dc4d, test_neon()); | ||
|
|
||
| // Conversions | ||
| test_utils::ops::test_cast!(u32x4 => f32x4, 0xcd18de89bb8b2fcf, test_neon()); |
There was a problem hiding this comment.
I know this seems kind of arbitrary, but since SIMDCast<f32> for u32x4 is implemented for the u32x4 type (i.e., it's on the right-hand side of the impl block), can test_cast!(u32x4 => f32x4, ...) go in u32x4_.rs?
What does this implement
Adds AArch64 NEON full-precision query-to-data kernels for 1/2/4-bit quantized inner product, wires spherical dispatch to use them directly.
Kernels written by MustafaIdrisArm
Any other comments?
I added a commit to avoid expanding diskann-wide with an interleaved trait, instead taking a inline helper functions approach in diskann-quantization. However if there is an argument for interleaved traits in diskann-wide then we can revert my last commit.