PiPNN 2/6: add numerical kernels - #1287
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the first set of PiPNN “kernel” building blocks to the DiskANN Rust workspace: SIMD-accelerated top‑k selection for partition assignment and leaf neighbor selection, along with supporting SIMD division and a new lower-triangular A·Aᵀ helper in diskann-linalg.
Changes:
- Add a new
diskann-pipnncrate withpartition_kernelandleaf_kernelimplementations plus extensive correctness tests and Criterion benchmarks. - Extend
diskann-wideto supportDivon relevant f32 SIMD types (native, doubled, and scalar/emulated) and add a corresponding division test macro. - Add
diskann_linalg::sgemm_aat_lower(lower-triangle-only AAT) and wire new crate/tests/CI/mutants exclusions into the workspace.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-wide/src/test_utils/ops.rs | Adds test_div! macro to validate lane-wise SIMD division correctness. |
| diskann-wide/src/emulated.rs | Adds Div for scalar/emulated Emulated<f32, N, A> to support division in scalar dispatch. |
| diskann-wide/src/doubled.rs | Adds Div for Doubled<T> to support composite SIMD widths. |
| diskann-wide/src/arch/x86_64/v4/f32x8_.rs | Adds AVX Div op mapping + division tests. |
| diskann-wide/src/arch/x86_64/v4/f32x4_.rs | Adds SSE Div op mapping + division tests. |
| diskann-wide/src/arch/x86_64/v4/f32x16_.rs | Adds AVX-512 Div op mapping + division tests. |
| diskann-wide/src/arch/x86_64/v3/f32x8_.rs | Adds AVX Div op mapping + division tests for V3. |
| diskann-wide/src/arch/x86_64/v3/f32x4_.rs | Adds SSE Div op mapping + division tests for V3. |
| diskann-wide/src/arch/x86_64/v3/f32x16_.rs | Adds division tests for the f32x16 V3 path (likely via doubled composition). |
| diskann-wide/src/arch/aarch64/f32x4_.rs | Adds Neon Div op mapping + division tests. |
| diskann-wide/src/arch/aarch64/f32x2_.rs | Adds Neon Div op mapping + division tests. |
| diskann-pipnn/tests/partition_kernel.rs | New integration tests for partition top‑k dispatch correctness and edge cases. |
| diskann-pipnn/tests/leaf_kernel.rs | New integration tests for leaf neighbor top‑k dispatch correctness and edge cases. |
| diskann-pipnn/src/partition_kernel/tests.rs | New unit tests comparing scalar reference vs runtime dispatch and metric contracts. |
| diskann-pipnn/src/partition_kernel.rs | New partition-assignment distance + top‑k kernel with validation and SIMD dispatch. |
| diskann-pipnn/src/lib.rs | New crate root exporting PiPNN kernel modules. |
| diskann-pipnn/src/leaf_kernel/tests.rs | New unit tests for scalar reference parity and workspace behavior. |
| diskann-pipnn/src/leaf_kernel.rs | New fused lower-triangle leaf neighbor kernel with SIMD dispatch and workspace support. |
| diskann-pipnn/Cargo.toml | Defines new diskann-pipnn crate, dev-deps, and benches. |
| diskann-pipnn/benches/kernels.rs | Adds benchmarks for partition top‑k, lower AAT, leaf top‑k, and full leaf workflow. |
| diskann-linalg/tests/sgemm_aat_lower.rs | New tests for lower-triangle AAT behavior and validation errors. |
| diskann-linalg/src/lib.rs | Adds public sgemm_aat_lower API with dimension checks. |
| diskann-linalg/src/faer.rs | Implements sgemm_aat_lower_impl using Faer triangular matmul. |
| Cargo.toml | Adds diskann-pipnn to workspace members and workspace dependencies. |
| Cargo.lock | Records the new diskann-pipnn package entry. |
| .github/workflows/ci.yml | Adds diskann-pipnn to CI test package lists. |
| .cargo/mutants.toml | Adds mutation-test exclusions for kernel code paths and equivalent transformations. |
Comments suppressed due to low confidence (2)
diskann-pipnn/src/leaf_kernel.rs:651
- Same issue as the L2 arm: using
max_simdfor lower clamping can erase NaNs on the Scalar/Emulated backend, making NaN distances rankable. Clamp withlt_simd+selectto preserve NaNs consistently.
Metric::CosineNormalized => {
let distance = F::splat(arch, 1.0) - dot;
zero.max_simd(distance)
}
diskann-pipnn/src/leaf_kernel.rs:664
- The cosine path also uses
zero.max_simd(distance)for clamping, which can collapse NaNs to zero on the Scalar/Emulated backend (viaf32::max). That contradicts the comment about preserving non-rankable NaNs and can change output ordering. Prefer anlt_simd+selectclamp here as well.
let distance = one - cosine;
// Comparisons with NaN are false, so this explicit lower clamp
// preserves non-rankable NaNs while matching the existing PiPNN
// distance formulas for finite values.
zero.max_simd(distance)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e204cb9 to
b046174
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pipnn-stack/02-final-prune #1287 +/- ##
===========================================================
Coverage 91.27% 91.28%
===========================================================
Files 517 517
Lines 98541 98624 +83
===========================================================
+ Hits 89945 90027 +82
- Misses 8596 8597 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
diskann-pipnn/src/partition_kernel/tests.rs:20
- The
PartitionTopKcontract forMetric::L2expectsleader_scalesto contain squared leader norms (see docs anddistance(Metric::L2, ..)test). This helper currently populates unsquared norms, which makes the test data inconsistent with the public API contract and could hide contract-related bugs.
let leader_scales = match metric {
Metric::L2 => (0..leaders).map(|leader| (leader + 1) as f32).collect(),
Metric::Cosine => (0..leaders)
.map(|leader| {
diskann-pipnn/src/partition_kernel.rs:61
InvalidFanout’s error message says the maximum is{maximum}, but validation also rejectsfanout > leaders. Whenleaders < maximumthis message is misleading (it implies the only limit is{maximum}). Consider spelling out both constraints in the message so callers immediately see why it failed.
#[error("invalid fanout {fanout} for {leaders} leaders; maximum is {maximum}")]
8fb4e92 to
20ab8a0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
diskann-pipnn/src/partition_kernel.rs:294
- For
Metric::Cosine, NaN norms currently produce a finite distance (1.0) becausedenominator.gt_simd(0)is false for NaN, so the lane falls back tocosine = 0. That makes NaN-derived pairs/leaders “rankable”, which contradicts the module’s stated NaN-rejection behavior and differs fromdiskann-vectorcosine semantics (NaN norms propagate to a NaN similarity/distance). Consider explicitly preserving NaN denominators so the resulting distance stays NaN and is ignored byinsert_topk.
let denominator = row_norm * leader_norm;
let valid = denominator.gt_simd(zero);
let safe_denominator = valid.select(denominator, one);
let cosine = valid.select(dot / safe_denominator, zero);
one - cosine
| check_length("leader scales", input.leader_scales.len(), leader_scales) | ||
| } | ||
|
|
||
| fn checked_area( |
There was a problem hiding this comment.
checked_area, check_length, ShapeOverflow and InvalidBufferLength are duplicated character-for-character with leaf_kernel. Small enough to shrug at now, but with four more PRs coming it's probably worth a src/shape.rs with a shared ShapeError that each kernel error wraps via #[from].
There was a problem hiding this comment.
I kept the two tiny checked-area/length adapters local because they construct different public kernel error types and sit immediately before each module's unsafe accesses. MatrixView adoption removed the other duplicated shape state; introducing a shared wrapped error would enlarge the public error interface for two call sites.
Aditya Krishnan (arkrishn94)
left a comment
There was a problem hiding this comment.
Thanks Weiyao, this is progress from the previous mega-PR. I still have some big-picture comments (we covered most of these offline) -
- Documentation: As I mentioned, we need thorough documentation in the
diskann-pipnncrate. The main modules,partition_kernelandleaf_kernelneed documentation up top, highlighting the main structures and how they are used - e.g.process_rows_binary/unaryandnearest_leaders. Similarly withprocess_pairs_simd_*andnearest_leaf_neighbors - Testing: I am concerned about the lack of testing for partition_kernel.rs and
leaf_kernel.rs.- I notice some e2e integration tests but these kernels should be thoroughly tested, sweeping different input parameters, architectures and edge cases. This is especially needed given the amount of unsafe code.
- That brings me to miri - there should be miri tests too.
- I'm curious why are the tests in a separate submodule to the main files (for partition_kernel.rs and leaf_kernel.rs)? Let's try to keep tests along with the code being tested.
- Criterion: Since criterion is not a standard part of our library for benchmarking, let us not introduce it for this crate.
- Kernel dispatch: I left comments about you're disptaching the kernels, please take a look.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (2)
diskann-pipnn/src/partition_kernel/tests.rs:19
PartitionTopK::leader_scalesis documented as "squared leader norms for L2" (and cosine uses unsquared norms), but this test helper feeds unsquared values for the L2 case. That makes the test data inconsistent with the public contract and can mask mistakes in distance computation. Consider squaring the L2 norms here so the tests exercise the intended inputs.
let leader_scales = match metric {
Metric::L2 => (0..leaders).map(|leader| (leader + 1) as f32).collect(),
Metric::Cosine => (0..leaders)
diskann-pipnn/src/partition_kernel.rs:252
- For the L2 path, the SIMD chunk uses
mul_add_simd(fused multiply-add) but the scalar tail usesnorm - 2.0 * dot(non-fused). This can introduce small rounding differences between SIMD and tail elements, which can change ordering/tie behavior right at SIMD-width boundaries. Usef32::mul_addfor the scalar tail so both paths compute the same value shape.
|dot, norm| F::splat(arch, -2.0).mul_add_simd(dot, norm),
|dot, norm| norm - 2.0 * dot,
@microsoft-github-policy-service agree company="Microsoft" |
Remove the submitted DiskANN microbenchmark target and co-locate numerical tests with their implementation files.
Add only the PiPNN feature and SDE package coverage; leave workflow triggers and existing formatting unchanged.
Establish matrix, scale, output, and scratch lengths in the safe functions that contain unchecked loads instead of relying only on upstream comments.
84dff03 to
7f94974
Compare
Remove the one-field LeafInput wrapper while keeping prepared metric and ISA dispatch unchanged.
|
Simplification follow-up in Validation: 33 kernel tests, all-target Clippy, and PiPNN doctests pass. |
Production uses fixed k=2/3. Reject larger widths and remove the duplicate dynamic storage and insertion path.
Keep supported fixed widths panic-free and remove internal unreachable branches.
Make every MatrixBase constructor preserve the backing-length invariant before PiPNN relies on MatrixView shape.
Remove duplicate backing-area checks and trim kernel module docs to contracts, invariants, and complexity.
Keep panic-free invalid-width handling while restoring the k=1/2/3 shifts that beat the generic bubble loop in Callgrind.
|
Follow-up simplification in
A generic fixed-row bubble insertion was measured and rejected: local Callgrind |
Use the existing f32x16 ConstLanes contract so PiPNN can read to_array results without changing diskann-wide traits.
04acfff to
d272fb9
Compare
Dispatch runtime convenience calls immediately, expose direct generic stage kernels, and replace the arbitrary fanout cap with caller-owned reusable tracker storage.
PiPNN (Pick-in-Partitions Nearest Neighbors) builds ANN graph candidates with overlapping partitions and dense matrix work instead of running beam search against a partially built graph for every inserted point. This numerical layer adds the kernels used by later PiPNN stages. It does not yet build or persist a graph.
PiPNN now lives under
diskann::graph::pipnn; there is no separate implementation crate. This keeps graph construction beside DiskANN graph policy and lets later layers reuse crate-private graph internals without publishing them.Concepts
fanoutis the number of nearest leaders retained for each point, creating overlapping child partitions.kis the number of local companions retained per point; it is construction policy, not final graph degreeR.Code map
diskann-linalg::sgemm_aat_lowercomputesA · Aᵀand writes only the lower triangle. Callers may leave the upper triangle uninitialized.diskann/src/graph/pipnn/kernel_metric.rsowns concrete metric marker types, formulas, scale units, and zero/NaN behavior shared by both kernels; runtime dispatch remains outside the numerical layer.partition_kernel.rsconverts point-by-leader dot products into sorted nearest leader IDs. Metric-specific scale handling happens before insertion into a caller-owned, runtime-sized tracker.leaf_kernel.rsscans each strict-lower-triangle pair once and updates both endpoint top-k trackers. It acceptsk <= 3and uses fixed-size insertion; larger widths are rejected.partition_kernel.rsandleaf_kernel.rsco-locate private seam tests with independent formula differentials;diskann-linalg/src/lib.rssimilarly owns the lower-triangle GEMM tests.End-to-end flow
Caller selects concrete architecture
Aand metric markerM→ computes dense dot products → typed kernel input validates matrix/scales/output → scalar/SIMD chunks convert dots to metric distances → stable top-k insertion writes caller-owned IDs/neighbors. #1290 performs that selection once for the complete graph build. This layer exposes no runtime kernel convenience wrapper, visitor, trait object, or stored function pointer.The kernels do not own providers, graph IDs, recursion, edge merging, thread pools, persistence, or search.
Invariants and boundaries
MatrixViewowns backing-shape validation; kernels validate cross-view row counts, scale layouts/lengths, fanout versus leader count, output width, and ID representation.u32positions; leaf output contains leaf-local target positions.f32::MAXremains rankable.diskann-wide.Review path
kernel_metric.rs: metric formulas, scale kinds, zero thresholds, NaN handling, and scalar equivalents.partition_kernel::nearest_leadersvalidation and tracker insertion, then compare scalar tails with SIMD chunks.leaf_kernel::nearest_neighborslower-triangle traversal, dual-endpoint updates, fixed-width top-k paths, and workspace reuse.sgemm_aat_lowernever touches the upper triangle.Validation
sgemm_aat_lower.k/fanout paths, dimensions around lane boundaries, tails, ties, NaN, infinities, signed zero, zero/singleton/capacity inputs, and validation failures.pipnn; existing feature-gated CI and nightly jobs includepipnn,testing.Stack relation
Stack 2/6. Depends on #1315, which extracts crate-private shared RobustPrune. This layer supplies numerical selection; #1290 adds partition/leaf orchestration using both lower layers.
Stack 2/6: #1315 → #1290