Skip to content

fix(linalg): stop FP16 hosts falling to scalar when the AVX-512 f16 kernel is absent - #8868

Open
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:fix/fp16-avx512-dispatch
Open

fix(linalg): stop FP16 hosts falling to scalar when the AVX-512 f16 kernel is absent#8868
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:fix/fp16-avx512-dispatch

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What this changes

Adds one match arm at each of eight f16 and bf16 dispatch sites, in cosine.rs, dot.rs, l2.rs and norm_l2.rs, so that an Avx512FP16 host uses the AVX2 kernel instead of scalar when the AVX-512 f16 kernel was not built. Found while reviewing #8866, which fixes the same shape in dist_table.rs.

The bug

Each site looks like this, and the fallback arm omits Avx512FP16:

#[cfg(all(feature = "fp16kernels", kernel_support = "avx512_bf16", target_arch = "x86_64"))]
SimdSupport::Avx512FP16 => ... l2_bf16_avx512 ...
#[cfg(all(feature = "fp16kernels", target_arch = "x86_64"))]
SimdSupport::Avx2 | SimdSupport::Avx512 => ... l2_bf16_avx2 ...
_ => l2_scalar::<Self, f32, 16>(x, y),

SIMD_SUPPORT is a single exclusive tier, so an FP16-capable host reports Avx512FP16 and never Avx2 or Avx512. When kernel_support = "avx512_bf16" is unset the first arm is compiled out, and that host then matches nothing and runs scalar bf16, even though the AVX2 kernel is present: build.rs builds it unconditionally on x86_64 and treats a failure there as a hard error, while the AVX-512 build only warns and continues.

The unset case is reachable. build.rs compiles the AVX-512 f16 C with -march=sapphirerapids, which needs clang 12 or gcc 11; older compilers get cargo:warning=Skipping build of AVX-512 fp16 kernels and the cfg stays unset.

Why a separate arm rather than widening the existing one

Widening does not compile. With the cfg set, the SimdSupport::Avx512FP16 => arm above has no guard, so it matches every value of that tier, and rustc rejects the addition at all eight sites:

error: unreachable pattern
121 |  SimdSupport::Avx512FP16 => unsafe {
    |  ----------------------- matches all the relevant values
125 |  SimdSupport::Avx2 | SimdSupport::Avx512 | SimdSupport::Avx512FP16
    |                                            ^^^^ no value can reach this

That is the difference from #8866, where the AVX-512 arm carries an if avx512bw guard and so is not exhaustive for its tier. So the new arm is gated on not(kernel_support = ...) and exists only in the configuration that has the bug.

The is_x86_feature_detected!("fma") guard is there because these kernels are compiled -march=haswell, which includes FMA, while has_avx512() is only is_x86_feature_detected!("avx512f") and never checks FMA. The precedent is l2.rs:342. Note the pre-existing arm below has the same unguarded exposure for the plain Avx512 tier; I left it alone to keep this change to the bug, and it is worth a separate look.

No test, and why

CLAUDE.md asks for a test with every bugfix, and there is none here. I do not think a host-independent one is constructible: the arm is selected by the SIMD_SUPPORT LazyLock static, which has no injection point, and by a build-time cfg, so neither input is reachable from a test on hardware that is not an FP16 AVX-512 part.

What does cover it, conditionally: test_l2_norm_f16 and _bf16, test_l2_distance_f16 and _bf16, test_dot_f16 and _bf16, test_cosine_f16 and _bf16 all assert dispatched-against-scalar parity at max_relative = 1e-3. On any host that takes the new arm those catch a wrong kernel or a wrong argument order. On every other host they pass without exercising it.

One more thing affected hosts will notice

The change is not only about speed. build.rs compiles these kernels with -ffast-math, so a host moving from the Rust scalar loop to the C AVX2 kernel gets a re-associated reduction and its distances shift, within the existing 1e-3 tolerance. The same shift already exists between Avx512 and Avx512FP16 hosts today, so this is not new for the codebase, but it is new for the hosts this unblocks: a recall number or a persisted distance measured on one of them is not bit-comparable afterwards.

Test plan

The load-bearing check is compiling the configuration that has the bug, which needs the cfg genuinely unset. I forced that by making both AVX-512 f16 builds in build.rs fail with a bogus -march, confirmed the two skip warnings appeared, and compiled. Doing that caught two errors in my first attempt, which had assumed every site takes (x, y): norm_l2 takes a single vector, and cosine takes (x, x_norm, y, len).

configuration result
default features, aarch64 and x86_64 clean
--features fp16kernels, aarch64 clean
--features fp16kernels, x86_64, cfg set clean, new arms compiled out
--features fp16kernels, x86_64, cfg unset clean, 2 skip warnings

cargo test --profile ci -p lance-linalg --lib, with and without fp16kernels: 389 passed, 1 ignored. cargo fmt --all -- --check clean.

What none of this covers: no runner here has AVX-512, so the new arm is compiled but never executed. It is also not unit-testable, because the match reads the SIMD_SUPPORT static and the cfg is decided at build time.

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer bug Something isn't working labels Aug 29, 2026

@lance-gatekeeper lance-gatekeeper Bot 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.

Gate recommendation: request changes.

The AVX2 fallback direction addresses the real scalar-dispatch bug, but this revision exposes safe callers to an unchecked FFI boundary and the exact cfg-unset path also violates existing f16 numerical tests. A viable revision should validate cosine lengths before every C dispatch, gate each fallback on the ISA actually emitted by its kernel, and make the newly selected AVX2 path satisfy the existing numerical contract.

target_arch = "x86_64"
))]
SimdSupport::Avx512FP16 if std::is_x86_feature_detected!("fma") => unsafe {
kernel::cosine_f16_avx2(x.as_ptr(), x_norm, y.as_ptr(), y.len() as u32)

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.

This new branch can read past x from safe Rust. Cosine::cosine_fast does not validate equal lengths, but cosine_f16_avx2 receives y.len() and its C loop reads both x[i] and y[i]. In the base cfg-unset path, cosine_scalar instead reaches the checked Dot::dot and panics. Please validate the lengths before dispatch in both the f16 and bf16 implementations so every unsafe C arm inherits the boundary check.

Reproducer run on this head

I forced both AVX-512 builds to fail with a CC wrapper that rejects -march=sapphirerapids, then added a test which mmaps two pages, protects the second with PROT_NONE, places a one-element f16 x at the end of the first page, and calls <f16 as Cosine>::cosine_fast(x, 1.0, &[1.0, 2.0]).

CC=/path/to/cc-no-spr CARGO_TARGET_DIR=/home/agent/tmp/gate-8868-target cargo test --profile ci -p lance-linalg --test gate_8868_cosine_oob --features fp16kernels -- --nocapture
... process didn't exit successfully ... (signal: 11, SIGSEGV: invalid memory reference)

not(kernel_support = "avx512_bf16"),
target_arch = "x86_64"
))]
SimdSupport::Avx512FP16 if std::is_x86_feature_detected!("fma") => unsafe {

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.

Checking only fma does not establish the instruction set required by these unsafe calls. Avx512FP16 detection proves only OS-usable AVX-512F plus the FP16 CPUID bit, while these fallback objects are compiled with -march=haswell: the bf16 object emits AVX2 (vpmovzxwd ymm) and the f16 object emits F16C (vcvtph2ps), in addition to FMA. A virtualized or future feature set can therefore select this arm without supporting the called instructions. Please gate bf16 on AVX2+FMA and f16 on F16C+FMA (and audit the complete emitted requirements), or compile against narrower explicit flags and centralize the tested capability predicate.

Verification run on this head
cc -std=c17 -O3 -ffast-math -funroll-loops -DSUFFIX=_gate -march=haswell -S -o - rust/lance-linalg/src/simd/f16.c | rg 'vcvtph2ps|vfmadd'
cc -std=c17 -O3 -ffast-math -funroll-loops -DSUFFIX=_gate -march=haswell -S -o - rust/lance-linalg/src/simd/bf16.c | rg 'vpmovzxwd|vfmadd'

Both commands produced the named instructions; lance-core/src/utils/cpu.rs does not test F16C or AVX2 when constructing Avx512FP16.

target_arch = "x86_64"
))]
SimdSupport::Avx512FP16 if std::is_x86_feature_detected!("fma") => unsafe {
kernel::norm_l2_f16_avx2(vector.as_ptr(), vector.len() as u32)

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.

The newly selected f16 AVX2 path does not satisfy the repository's existing numerical tests on the exact target configuration. On this Sapphire Rapids host, forcing the AVX-512 builds to fail made the full library suite execute these cfg-unset arms: test_l2_distance_f16 and test_l2_norm_f16 failed their max_relative = 1e-6 assertions (450 passed; 2 failed; 1 ignored). Replaying the persisted cases with fp16kernels disabled took the prior scalar route and both passed. Please make the fallback meet the existing numerical contract, or otherwise establish and test an explicitly accepted result contract before routing these hosts to it.

Reproducer

The wrapper delegates to cc except that it exits unsuccessfully for -march=sapphirerapids, leaving kernel_support="avx512_f16" and kernel_support="avx512_bf16" genuinely unset.

CC=/path/to/cc-no-spr CARGO_TARGET_DIR=/home/agent/tmp/gate-8868-target cargo test --profile ci -p lance-linalg --lib --features fp16kernels

Observed examples were L2 439198480000.0 versus 439199070000.0, and norm-L2 621139.2 versus 621139.8.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Aug 29, 2026
@LuciferYang
LuciferYang force-pushed the fix/fp16-avx512-dispatch branch from 177cfcd to 6bc9b11 Compare August 29, 2026 15:27
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Aug 29, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Aug 29, 2026
@LuciferYang
LuciferYang force-pushed the fix/fp16-avx512-dispatch branch from 6bc9b11 to bdb71ff Compare August 29, 2026 16:18
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Aug 29, 2026

@lance-gatekeeper lance-gatekeeper Bot 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.

Gate recommendation: request changes.

0 fixed / 3 remain. This revision rebases the same patch without changing the affected linalg code. The safe FFI boundary, complete ISA predicate, and existing f16 numerical contract still need to be addressed before the AVX2 fallback can be accepted.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer bug Something isn't working K-changes Latest Gatekeeper recommendation requests changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant