Skip to content

PiPNN 2/6: add numerical kernels - #1287

Open
weiyaoluo (SeliMeli) wants to merge 68 commits into
pipnn-stack/02-final-prunefrom
pipnn-stack/01-kernels
Open

PiPNN 2/6: add numerical kernels#1287
weiyaoluo (SeliMeli) wants to merge 68 commits into
pipnn-stack/02-final-prunefrom
pipnn-stack/01-kernels

Conversation

@SeliMeli

@SeliMeli weiyaoluo (SeliMeli) commented Jul 29, 2026

Copy link
Copy Markdown

Purpose

This PR adds the numerical kernels that PiPNN uses for partition ranking and leaf neighbor selection.

It does not add graph construction, providers, or serialization.

Main changes

  • sgemm_aat_lower computes the diagonal and lower triangle of A · Aᵀ.
  • PartitionKernelMetric defines partition ranking formulas.
  • LeafKernelMetric defines leaf distance formulas.
  • Shared functions handle cosine distance and squared-norm conversion.
  • partition_kernel supports runtime fanout with reusable workspace.
  • leaf_kernel scans each unordered pair once and supports k values from 1 through 3.
  • diskann-wide adds SIMD division for supported f32 vector types.
  • MatrixView rejects an overflowing declared area.

ScaleKind does not exist. All norm names state their units.

Numerical behavior

  • Strict comparisons preserve scan order for ties.
  • Kernel comparisons do not rank NaN.
  • L2 leaf distances preserve NaN during the zero clamp.
  • L2 partition SIMD groups use fused arithmetic.
  • The L2 scalar tail uses non-fused arithmetic.
  • Cosine preserves the DiskANN zero-norm rules.

This PR contains three temporary allow(dead_code) attributes. #1290 adds production callers and removes all three attributes.

Review order

  1. Review diskann/src/graph/pipnn/kernel_metric.rs.
  2. Review kernel_metric/partition.rs.
  3. Review partition_kernel.rs.
  4. Review kernel_metric/leaf.rs.
  5. Review leaf_kernel.rs.
  6. Review sgemm_aat_lower and SIMD division.

Validation

  • 32 kernel tests cover all metrics and SIMD boundaries.
  • Tests cover 15, 16, 17, 31, 32, and 33 leaders or points.
  • A 17-leader L2 test proves that the scalar tail can outrank a fused SIMD lane.
  • Linux, Windows GNU, and AArch64 Clippy pass with -Dwarnings.
  • This PR adds no submitted PiPNN benchmark target.

Stack

Stack 2/6. Depends on #1315. #1290 adds the production callers.

@SeliMeli
weiyaoluo (SeliMeli) requested review from a team and a lite review from Copilot July 29, 2026 11:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-pipnn crate with partition_kernel and leaf_kernel implementations plus extensive correctness tests and Criterion benchmarks.
  • Extend diskann-wide to support Div on 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_simd for lower clamping can erase NaNs on the Scalar/Emulated backend, making NaN distances rankable. Clamp with lt_simd + select to 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 (via f32::max). That contradicts the comment about preserving non-rankable NaNs and can change output ordering. Prefer an lt_simd + select clamp 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.

Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel/tests.rs Outdated
@SeliMeli weiyaoluo (SeliMeli) changed the title Pipnn stack/01 kernels PiPNN 1/6: add numerical kernels Jul 29, 2026
Copilot AI review requested due to automatic review settings July 30, 2026 08:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.32%. Comparing base (d230a2c) to head (df265f4).

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##           pipnn-stack/02-final-prune    #1287      +/-   ##
==============================================================
+ Coverage                       91.31%   91.32%   +0.01%     
==============================================================
  Files                             517      517              
  Lines                           98513    98591      +78     
==============================================================
+ Hits                            89955    90043      +88     
+ Misses                           8558     8548      -10     
Flag Coverage Δ
miri 91.32% <100.00%> (+0.01%) ⬆️
unittests 91.00% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
diskann-linalg/src/faer.rs 100.00% <100.00%> (ø)
diskann-linalg/src/lib.rs 99.72% <100.00%> (+0.03%) ⬆️
diskann-utils/src/views.rs 100.00% <100.00%> (ø)
diskann-wide/src/arch/x86_64/v3/f32x16_.rs 100.00% <ø> (ø)
diskann-wide/src/arch/x86_64/v3/f32x4_.rs 100.00% <ø> (ø)
diskann-wide/src/arch/x86_64/v3/f32x8_.rs 100.00% <ø> (ø)
diskann-wide/src/arch/x86_64/v4/f32x16_.rs 14.11% <ø> (ø)
diskann-wide/src/arch/x86_64/v4/f32x4_.rs 16.90% <ø> (ø)
diskann-wide/src/arch/x86_64/v4/f32x8_.rs 16.90% <ø> (ø)
diskann-wide/src/doubled.rs 86.89% <100.00%> (+0.17%) ⬆️
... and 2 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings July 30, 2026 08:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PartitionTopK contract for Metric::L2 expects leader_scales to contain squared leader norms (see docs and distance(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 rejects fanout > leaders. When leaders < maximum this 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}")]

Copilot AI review requested due to automatic review settings July 31, 2026 04:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) because denominator.gt_simd(0) is false for NaN, so the lane falls back to cosine = 0. That makes NaN-derived pairs/leaders “rankable”, which contradicts the module’s stated NaN-rejection behavior and differs from diskann-vector cosine semantics (NaN norms propagate to a NaN similarity/distance). Consider explicitly preserving NaN denominators so the resulting distance stays NaN and is ignored by insert_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

Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann/src/graph/pipnn/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated

@partychen juchen-ms (partychen) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work overall. I found one correctness issue in the cosine handling that should be resolved before merge. The remaining comments are mostly about reducing duplicated or unsafe code and tightening the API contracts.

Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann/src/graph/pipnn/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-linalg/src/lib.rs Outdated
Comment thread diskann-wide/src/emulated.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-pipnn crate. The main modules, partition_kernel and leaf_kernel need documentation up top, highlighting the main structures and how they are used - e.g. process_rows_binary/unary and nearest_leaders. Similarly with process_pairs_simd_* and nearest_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.

Comment thread .cargo/mutants.toml Outdated
Comment thread diskann-pipnn/src/lib.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/src/partition_kernel.rs Outdated
Comment thread diskann-pipnn/tests/leaf_kernel_api.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Comment thread diskann-pipnn/src/leaf_kernel.rs Outdated
Copilot AI review requested due to automatic review settings August 3, 2026 02:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_scales is 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 uses norm - 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. Use f32::mul_add for 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,

@SeliMeli

Copy link
Copy Markdown
Author

weiyaoluo (@SeliMeli) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

Copilot AI review requested due to automatic review settings August 3, 2026 10:49
@SeliMeli weiyaoluo (SeliMeli) changed the title PiPNN 2/6: add numerical kernels PiPNN 2/6: add numerical foundations Aug 10, 2026
@SeliMeli

Copy link
Copy Markdown
Author

The PR boundary changed to remove dead-code suppression: PR2 now contains only the called linalg, SIMD, and MatrixView foundations. PiPNN metric/partition/leaf kernels, their co-located tests, feature wiring, and CI coverage moved to #1290 with their real callers. The former kernel threads are therefore outdated; their fixes remain in #1290 (runtime fanout, row_iter, build-wide dispatch, no IAI target, no duplicate matrix backing checks, and no production panic path). I am resolving the outdated threads here.

@SeliMeli weiyaoluo (SeliMeli) changed the title PiPNN 2/6: add numerical foundations PiPNN 2/6: add numerical kernels Aug 10, 2026
@SeliMeli

Copy link
Copy Markdown
Author

Restored the intended stack boundary: kernel_metric, leaf_kernel, and partition_kernel are again part of PR2. This intermediate layer has three explicit temporary allow(dead_code) attributes because the crate-private kernels intentionally land before their production caller; no fake call or public API was added.

@SeliMeli

Copy link
Copy Markdown
Author

Refactored the kernel metric layer. ScaleKind and all scale terminology are removed. LeafKernelMetric and PartitionKernelMetric now own separate formulas; MetricTag, cosine math, and squared-norm conversion remain shared. Partition methods now use partition_ranking names. A 17-leader test covers fused SIMD versus non-fused scalar-tail ordering. New comments use short active STE100-style sentences.

@SeliMeli

Copy link
Copy Markdown
Author

Verified the earlier concern about small synthetic fixtures. It was valid for the reviewed revision, but the current tests cover the SIMD loops. Leaf tests use 15/16/17, 31/32/33, 64, 256, and 512 points for every metric and k=1/2/3. Partition tests use 15/16/17 and 31/32/33 leaders for every metric and runtime fanout. The 17-leader L2 test also distinguishes the fused SIMD block from the non-fused scalar tail. Targeted llvm-cov confirms execution of leaf_kernel.rs:326-345 and partition_kernel.rs:285-299. CI runs the same tests under SDE baseline and AVX-512 targets.

@SeliMeli

Copy link
Copy Markdown
Author

Follow-up: removed the separate MetricTag trait. PartitionKernelMetric now owns the metric identity because partition preparation and finalization need it. LeafKernelMetric contains only leaf norm and distance behavior.

@SeliMeli

Copy link
Copy Markdown
Author

Local Callgrind A/B against the previous kernel code: the isolated 64-point L2 leaf scan changed from 99,194,000 to 99,328,000 instructions (+0.14%). The 50-build L2 fixture changed from 77,305,792 to 77,323,007 (+0.02%). The matching cosine build fixture changed from 82,827,064 to 82,369,484 (-0.55%). Graph output remained byte-identical for all four metrics in the fixed-seed 64-point comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants