From 246dd131171fa6eee72e63b0949cc3e8b4f7b0d1 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 14:14:27 +0300 Subject: [PATCH 01/14] Give ARM kernel instantiations tier-local linkage Two tier translation units that include the same kernel header emitted the same weak (COMDAT) symbols, compiled under different -march flags. The linker then kept one body and discarded the other, chosen by link order, with nothing in the source deciding which. For each of the eight ARM tier TUs (NEON, NEON_DOTPROD, NEON_HP, NEON_FHM, NEON_BF16, SVE, SVE2, SVE_BF16), wrap the kernel-header includes in an anonymous namespace so the kernel instantiations get internal linkage, unique to each translation unit. Only the Choose_* entry points, still declared in the tier header, keep external linkage. The shared dependencies (space_includes.h, spaces.h, the type headers, and the ARM intrinsics headers) are hoisted above the anonymous namespace: wrapping them along with the kernel includes pulls the standard library into the anonymous namespace and fails to compile. Measured on arm-r8g.xlarge (gcc 12) with nm -g --defined-only before this change: NEON and NEON_DOTPROD shared 93 externally-defined symbols, SVE and SVE2 shared 166. This change makes the whole class of collision structurally impossible rather than fixing only those two known pairs. x86 was measured at 0 shared symbols across all 15 tier objects, every pair, because x86 kernel names embed the ISA and no x86 tier TU reuses another tier's kernel headers. x86 tier TUs are therefore untouched. Verification of the after state (all 28 ARM pairs sharing zero symbols, and every object still exporting its own Choose_* entry points) requires building on real ARM hardware and was not run as part of this change. --- src/VecSim/spaces/functions/NEON.cpp | 16 ++++++++++++++++ src/VecSim/spaces/functions/NEON_BF16.cpp | 15 +++++++++++++++ src/VecSim/spaces/functions/NEON_DOTPROD.cpp | 16 ++++++++++++++++ src/VecSim/spaces/functions/NEON_FHM.cpp | 15 +++++++++++++++ src/VecSim/spaces/functions/NEON_HP.cpp | 15 +++++++++++++++ src/VecSim/spaces/functions/SVE.cpp | 16 ++++++++++++++++ src/VecSim/spaces/functions/SVE2.cpp | 15 +++++++++++++++ src/VecSim/spaces/functions/SVE_BF16.cpp | 15 +++++++++++++++ 8 files changed, 123 insertions(+) diff --git a/src/VecSim/spaces/functions/NEON.cpp b/src/VecSim/spaces/functions/NEON.cpp index 0c9a286e3..8d5c286be 100644 --- a/src/VecSim/spaces/functions/NEON.cpp +++ b/src/VecSim/spaces/functions/NEON.cpp @@ -7,6 +7,21 @@ * GNU Affero General Public License v3 (AGPLv3). */ #include "NEON.h" + +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/L2/L2_NEON_FP32.h" #include "VecSim/spaces/IP/IP_NEON_FP32.h" #include "VecSim/spaces/L2/L2_NEON_INT8.h" @@ -19,6 +34,7 @@ #include "VecSim/spaces/IP/IP_NEON_SQ8_FP32.h" #include "VecSim/spaces/IP/IP_NEON_SQ8_SQ8.h" #include "VecSim/spaces/L2/L2_NEON_SQ8_SQ8.h" +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/NEON_BF16.cpp b/src/VecSim/spaces/functions/NEON_BF16.cpp index 4de205bb8..1a515943e 100644 --- a/src/VecSim/spaces/functions/NEON_BF16.cpp +++ b/src/VecSim/spaces/functions/NEON_BF16.cpp @@ -8,8 +8,23 @@ */ #include "NEON_BF16.h" +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/L2/L2_NEON_BF16.h" #include "VecSim/spaces/IP/IP_NEON_BF16.h" +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/NEON_DOTPROD.cpp b/src/VecSim/spaces/functions/NEON_DOTPROD.cpp index 12f762093..ade17bb31 100644 --- a/src/VecSim/spaces/functions/NEON_DOTPROD.cpp +++ b/src/VecSim/spaces/functions/NEON_DOTPROD.cpp @@ -7,12 +7,28 @@ * GNU Affero General Public License v3 (AGPLv3). */ #include "NEON.h" + +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/IP/IP_NEON_DOTPROD_INT8.h" #include "VecSim/spaces/IP/IP_NEON_DOTPROD_UINT8.h" #include "VecSim/spaces/IP/IP_NEON_DOTPROD_SQ8_SQ8.h" #include "VecSim/spaces/L2/L2_NEON_DOTPROD_INT8.h" #include "VecSim/spaces/L2/L2_NEON_DOTPROD_UINT8.h" #include "VecSim/spaces/L2/L2_NEON_DOTPROD_SQ8_SQ8.h" +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/NEON_FHM.cpp b/src/VecSim/spaces/functions/NEON_FHM.cpp index 2beceed0c..ed92bec47 100644 --- a/src/VecSim/spaces/functions/NEON_FHM.cpp +++ b/src/VecSim/spaces/functions/NEON_FHM.cpp @@ -8,8 +8,23 @@ */ #include "NEON_FHM.h" +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/IP/IP_NEON_SQ8_FP16.h" #include "VecSim/spaces/L2/L2_NEON_SQ8_FP16.h" +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/NEON_HP.cpp b/src/VecSim/spaces/functions/NEON_HP.cpp index 20d93a517..412963066 100644 --- a/src/VecSim/spaces/functions/NEON_HP.cpp +++ b/src/VecSim/spaces/functions/NEON_HP.cpp @@ -8,10 +8,25 @@ */ #include "NEON_HP.h" +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/L2/L2_NEON_FP16.h" #include "VecSim/spaces/IP/IP_NEON_FP16.h" #include "VecSim/spaces/IP/IP_NEON_SQ8_FP16.h" #include "VecSim/spaces/L2/L2_NEON_SQ8_FP16.h" +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/SVE.cpp b/src/VecSim/spaces/functions/SVE.cpp index bd197c84c..e306accbb 100644 --- a/src/VecSim/spaces/functions/SVE.cpp +++ b/src/VecSim/spaces/functions/SVE.cpp @@ -8,6 +8,21 @@ */ #include "SVE.h" +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/L2/L2_SVE_FP32.h" #include "VecSim/spaces/IP/IP_SVE_FP32.h" @@ -30,6 +45,7 @@ #include "VecSim/spaces/IP/IP_SVE_SQ8_SQ8.h" #include "VecSim/spaces/L2/L2_SVE_SQ8_SQ8.h" +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/SVE2.cpp b/src/VecSim/spaces/functions/SVE2.cpp index 9eea81523..d15780d73 100644 --- a/src/VecSim/spaces/functions/SVE2.cpp +++ b/src/VecSim/spaces/functions/SVE2.cpp @@ -8,6 +8,20 @@ */ #include "SVE2.h" +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/L2/L2_SVE_FP32.h" #include "VecSim/spaces/IP/IP_SVE_FP32.h" @@ -26,6 +40,7 @@ #include "VecSim/spaces/L2/L2_SVE2_SQ8_FP16.h" // SVE2 fast path: FMLALB/FMLALT widening #include "VecSim/spaces/IP/IP_SVE_SQ8_SQ8.h" // SVE2 implementation is identical to SVE #include "VecSim/spaces/L2/L2_SVE_SQ8_SQ8.h" // SVE2 implementation is identical to SVE +} // namespace namespace spaces { diff --git a/src/VecSim/spaces/functions/SVE_BF16.cpp b/src/VecSim/spaces/functions/SVE_BF16.cpp index b457cdb7f..d7b9a6994 100644 --- a/src/VecSim/spaces/functions/SVE_BF16.cpp +++ b/src/VecSim/spaces/functions/SVE_BF16.cpp @@ -8,8 +8,23 @@ */ #include "SVE_BF16.h" +// Hoisted above the anonymous namespace below so that the standard library and the shared +// type headers keep external linkage. Wrapping them would pull and friends into +// the anonymous namespace and fail to compile. +#include "VecSim/spaces/space_includes.h" +#include "VecSim/spaces/spaces.h" +#include "VecSim/types/bfloat16.h" +#include "VecSim/types/float16.h" +#include "VecSim/types/sq8.h" +#include + +// Kernel instantiations get internal linkage, unique to this translation unit, so two tiers +// that share a kernel header cannot emit the same weak symbol and let link order pick the +// body. Only this tier's Choose_* entry points stay external. +namespace { #include "VecSim/spaces/IP/IP_SVE_BF16.h" #include "VecSim/spaces/L2/L2_SVE_BF16.h" +} // namespace namespace spaces { From 0d42fb4f9c6140956eddba55a3598db88e2b54b7 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 18:00:34 +0300 Subject: [PATCH 02/14] Mirror the NEON_FHM dispatch predicate in its unit-test guards The three SQ8_FP16 optimization tests gated their FHM branch on optimization.asimdfhm alone, while the dispatcher now requires features.asimdhp && features.asimdfhm. The benchmark registrations already match the dispatcher; these three did not. Harmless in practice, since no core reports asimdfhm without asimdhp, but a test whose guard is looser than the code it tests will not catch the case it looks like it covers. --- tests/unit/test_spaces.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 18ef7a625..96456a9c5 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -3372,7 +3372,7 @@ TEST_P(SQ8_FP16_SpacesOptimizationTest, SQ8_FP16_L2SqrTest) { } #endif #ifdef OPT_NEON_FHM - if (optimization.asimdfhm) { + if (optimization.asimdhp && optimization.asimdfhm) { unsigned char alignment = 0; arch_opt_func = L2_SQ8_FP16_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_FP16_L2_implementation_NEON_FHM(dim)) @@ -3498,7 +3498,7 @@ TEST_P(SQ8_FP16_SpacesOptimizationTest, SQ8_FP16_InnerProductTest) { } #endif #ifdef OPT_NEON_FHM - if (optimization.asimdfhm) { + if (optimization.asimdhp && optimization.asimdfhm) { unsigned char alignment = 0; arch_opt_func = IP_SQ8_FP16_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_FP16_IP_implementation_NEON_FHM(dim)) @@ -3624,7 +3624,7 @@ TEST_P(SQ8_FP16_SpacesOptimizationTest, SQ8_FP16_CosineTest) { } #endif #ifdef OPT_NEON_FHM - if (optimization.asimdfhm) { + if (optimization.asimdhp && optimization.asimdfhm) { unsigned char alignment = 0; arch_opt_func = Cosine_SQ8_FP16_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_FP16_Cosine_implementation_NEON_FHM(dim)) From 9963f92cd789ce7f1fcbcddc85b49523c45d6faf Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 13:49:26 +0300 Subject: [PATCH 03/14] Gate the AVX512 VNNI tier on avx512vl at every call site The AVX512F_BW_VL_VNNI tier is compiled with -mavx512vl, but 6 of 12 runtime predicate sites omitted the avx512vl check, while 6 included it. A CPU with avx512f, avx512bw, and avx512vnni but without avx512vl would therefore be handed a function pointer into a TU the compiler was licensed to emit VL-encoded instructions in. Add avx512vl to all 12 sites, making them consistent. --- src/VecSim/spaces/IP_space.cpp | 10 ++++++---- src/VecSim/spaces/L2_space.cpp | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/VecSim/spaces/IP_space.cpp b/src/VecSim/spaces/IP_space.cpp index b7f963d2c..d13cc1fc2 100644 --- a/src/VecSim/spaces/IP_space.cpp +++ b/src/VecSim/spaces/IP_space.cpp @@ -77,7 +77,7 @@ dist_func_t IP_SQ8_FP32_GetDistFunc(size_t dim, unsigned char *alignment, } // Alignment hints below refer to the SQ8 (first) operand per the GetDistFunc contract. #ifdef OPT_AVX512_F_BW_VL_VNNI - if (features.avx512f && features.avx512bw && features.avx512vnni) { + if (features.avx512f && features.avx512bw && features.avx512vl && features.avx512vnni) { if (dim % 16 == 0) // SQ8 chunk = 16 bytes *alignment = 16 * sizeof(uint8_t); return Choose_SQ8_FP32_IP_implementation_AVX512F_BW_VL_VNNI(dim); @@ -146,7 +146,7 @@ dist_func_t Cosine_SQ8_FP32_GetDistFunc(size_t dim, unsigned char *alignm } // Alignment hints below refer to the SQ8 (first) operand per the GetDistFunc contract. #ifdef OPT_AVX512_F_BW_VL_VNNI - if (features.avx512f && features.avx512bw && features.avx512vnni) { + if (features.avx512f && features.avx512bw && features.avx512vl && features.avx512vnni) { if (dim % 16 == 0) // SQ8 chunk = 16 bytes *alignment = 16 * sizeof(uint8_t); return Choose_SQ8_FP32_Cosine_implementation_AVX512F_BW_VL_VNNI(dim); @@ -375,7 +375,8 @@ dist_func_t IP_SQ8_SQ8_GetDistFunc(size_t dim, unsigned char *alignment, #ifdef CPU_FEATURES_ARCH_X86_64 #ifdef OPT_AVX512_F_BW_VL_VNNI // AVX512 VNNI SQ8_SQ8 uses 64-element chunks; residual handling is in 32-byte sub-chunks. - if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vl && + features.avx512vnni) { if (dim % 32 == 0) // align to 256 bits when there is no offsetting residual *alignment = 32 * sizeof(uint8_t); return Choose_SQ8_SQ8_IP_implementation_AVX512F_BW_VL_VNNI(dim); @@ -427,7 +428,8 @@ dist_func_t Cosine_SQ8_SQ8_GetDistFunc(size_t dim, unsigned char *alignme #ifdef CPU_FEATURES_ARCH_X86_64 #ifdef OPT_AVX512_F_BW_VL_VNNI // AVX512 VNNI SQ8_SQ8 uses 64-element chunks; residual handling is in 32-byte sub-chunks. - if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vl && + features.avx512vnni) { if (dim % 32 == 0) // align to 256 bits when there is no offsetting residual *alignment = 32 * sizeof(uint8_t); return Choose_SQ8_SQ8_Cosine_implementation_AVX512F_BW_VL_VNNI(dim); diff --git a/src/VecSim/spaces/L2_space.cpp b/src/VecSim/spaces/L2_space.cpp index 2c9223b67..bca916ab3 100644 --- a/src/VecSim/spaces/L2_space.cpp +++ b/src/VecSim/spaces/L2_space.cpp @@ -76,7 +76,7 @@ dist_func_t L2_SQ8_FP32_GetDistFunc(size_t dim, unsigned char *alignment, } // Alignment hints below refer to the SQ8 (first) operand per the GetDistFunc contract. #ifdef OPT_AVX512_F_BW_VL_VNNI - if (features.avx512f && features.avx512bw && features.avx512vnni) { + if (features.avx512f && features.avx512bw && features.avx512vl && features.avx512vnni) { if (dim % 16 == 0) // SQ8 chunk = 16 bytes; no point in aligning if there's a residual *alignment = 16 * sizeof(uint8_t); return Choose_SQ8_FP32_L2_implementation_AVX512F_BW_VL_VNNI(dim); @@ -563,7 +563,8 @@ dist_func_t L2_SQ8_SQ8_GetDistFunc(size_t dim, unsigned char *alignment, #ifdef CPU_FEATURES_ARCH_X86_64 #ifdef OPT_AVX512_F_BW_VL_VNNI // AVX512 VNNI SQ8_SQ8 uses 64-element chunks; residual handling is in 32-byte sub-chunks. - if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vnni) { + if (dim >= 64 && features.avx512f && features.avx512bw && features.avx512vl && + features.avx512vnni) { if (dim % 32 == 0) // align to 256 bits when there is no offsetting residual *alignment = 32 * sizeof(uint8_t); return Choose_SQ8_SQ8_L2_implementation_AVX512F_BW_VL_VNNI(dim); From 25b6178fdf05d8b46a2e6abba32e84d3c3d5b07b Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 13:49:32 +0300 Subject: [PATCH 04/14] Gate test and benchmark ARM sources on the target CPU, not the host The spaces/CMakeLists.txt correctly uses CMAKE_SYSTEM_PROCESSOR for the target machine, but tests/unit/CMakeLists.txt and tests/benchmark/CMakeLists.txt used CMAKE_HOST_SYSTEM_PROCESSOR, which breaks cross-compilation: a build machine with AVX512 targeting ARM would incorrectly pull in x86-specific instruction flags. Switch both to CMAKE_SYSTEM_PROCESSOR. --- tests/benchmark/CMakeLists.txt | 2 +- tests/unit/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/benchmark/CMakeLists.txt b/tests/benchmark/CMakeLists.txt index 2cf536d91..db74b3023 100644 --- a/tests/benchmark/CMakeLists.txt +++ b/tests/benchmark/CMakeLists.txt @@ -27,7 +27,7 @@ endforeach() include(GoogleTest) -if (CMAKE_HOST_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv.*)") +if (CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv.*)") include(${root}/cmake/aarch64InstructionFlags.cmake) add_compile_definitions(BENCHMARK_ARCH=arm64) else() diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index c3e1cc987..d3a18221a 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -28,7 +28,7 @@ if(FP64_TESTS) add_definitions(-DFP64_TESTS) endif() -if (CMAKE_HOST_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv8)|(armv9)") +if (CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv8)|(armv9)") include(${root}/cmake/aarch64InstructionFlags.cmake) else() include(${root}/cmake/x86_64InstructionFlags.cmake) From 1e3793d6a4da56e48493678036dd71a1496087ba Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 14:33:08 +0300 Subject: [PATCH 05/14] Intersect an injected CPU-feature mask with the detected hardware getCpuOptimizationFeatures() let a caller-supplied arch_opt override replace the detected CPU features outright, so a caller could claim a feature the real hardware lacks and be handed a function pointer into a SIMD tier that executes an illegal instruction. The override is a test-only affordance: no production caller passes a non-null arch_opt (every chooser in spaces.cpp uses the nullptr default), tests use it to force a specific tier. Every existing test only ever clears bits (232 clears in test_spaces.cpp, zero sets), so intersecting the override with the detected features instead of replacing them is a no-op for the whole existing suite while closing the hole for any future or malformed test. The new intersectWithDetectedFeatures() helper performs a byte-wise AND over the two structs' object representation rather than hand-listing fields, since cpu_features::X86Features and cpu_features::Aarch64Features are composed entirely of `int : 1;` bitfields with no other members: a byte-wise AND therefore performs a per-feature logical AND, and hand-writing per-field assignments would rot the moment cpu_features adds a field. A static_assert guards the trivially-copyable assumption the memcpy-based AND relies on. The new CpuFeatureIntersectionX86 / CpuFeatureIntersectionAarch64 tests in test_spaces.cpp enforce the pure-bitfield layout assumption itself: they walk every enum value via cpu_features' own GetX86FeaturesEnumValue / GetAarch64FeaturesEnumValue readers and compare against the field-wise AND, so a future non-bitfield member would make the byte-wise AND diverge and fail loudly. They also cover the two cases called out by the task: clearing one feature out of an all-detected mask leaves every other feature unchanged, and claiming a feature the detected set lacks still comes out clear. --- src/VecSim/spaces/spaces.h | 39 +++++++++++++++- tests/unit/test_spaces.cpp | 94 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/src/VecSim/spaces/spaces.h b/src/VecSim/spaces/spaces.h index 735e40e89..273ae51f2 100644 --- a/src/VecSim/spaces/spaces.h +++ b/src/VecSim/spaces/spaces.h @@ -13,7 +13,9 @@ #include #include +#include #include +#include namespace spaces { @@ -65,6 +67,38 @@ static constexpr size_t UINT8_MAX_EXACT_SIMD_DIM = std::numeric_limits::max() / (std::numeric_limits::max() * std::numeric_limits::max()); +// Intersect an overridden feature mask with the features actually detected on this machine, so +// that the override can only clear a capability, never claim one the hardware does not have. +// +// `arch_opt` (below) is a test-only affordance: no production caller passes a non-null value +// (every chooser in spaces.cpp calls getCpuOptimizationFeatures() with the `= nullptr` default), +// tests use it to force a specific SIMD tier onto whatever machine runs the suite. Without this +// intersection, a test that claims a feature the real CPU lacks would hand back a function +// pointer into a tier that executes an illegal instruction. +// +// This is sound only because cpu_features::X86Features and cpu_features::Aarch64Features are +// composed entirely of `int : 1;` bitfields with no other members, so a byte-wise AND of +// the two object representations performs a per-feature logical AND. The +// CpuFeatureIntersection.* tests in test_spaces.cpp walk every feature via cpu_features' own +// GetX86FeaturesEnumValue / GetAarch64FeaturesEnumValue readers and enforce that layout +// assumption, so this breaks loudly if cpu_features ever adds a non-bitfield member. +template +static inline FeaturesType intersectWithDetectedFeatures(const FeaturesType &overridden, + const FeaturesType &detected) { + static_assert(std::is_trivially_copyable_v, + "byte-wise AND requires a trivially copyable features type"); + unsigned char overridden_bytes[sizeof(FeaturesType)]; + unsigned char detected_bytes[sizeof(FeaturesType)]; + std::memcpy(overridden_bytes, &overridden, sizeof(FeaturesType)); + std::memcpy(detected_bytes, &detected, sizeof(FeaturesType)); + for (size_t i = 0; i < sizeof(FeaturesType); i++) { + overridden_bytes[i] &= detected_bytes[i]; + } + FeaturesType result; + std::memcpy(&result, overridden_bytes, sizeof(FeaturesType)); + return result; +} + static inline auto getCpuOptimizationFeatures(const void *arch_opt = nullptr) { #if defined(CPU_FEATURES_ARCH_AARCH64) @@ -74,7 +108,10 @@ static inline auto getCpuOptimizationFeatures(const void *arch_opt = nullptr) { using FeaturesType = cpu_features::X86Features; // Fallback constexpr auto getFeatures = cpu_features::GetX86Info; #endif - return arch_opt ? *static_cast(arch_opt) : getFeatures().features; + const FeaturesType detected = getFeatures().features; + return arch_opt ? intersectWithDetectedFeatures(*static_cast(arch_opt), + detected) + : detected; } } // namespace spaces diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 96456a9c5..2b2fe7c7f 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -4915,3 +4915,97 @@ TEST_F(SpacesTest, SQ8_SQ8_DispatcherAlignmentHints) { check("Cosine", &spaces::Cosine_SQ8_SQ8_GetDistFunc); } #endif // CPU_FEATURES_ARCH_X86_64 + +// Enforce the pure-bitfield layout assumption that spaces::intersectWithDetectedFeatures (see +// spaces/spaces.h) relies on: the platform's cpu_features Features struct must be composed +// entirely of `int : 1;` bitfields with no other members, so a byte-wise AND over the +// struct's object representation is equivalent to a per-feature logical AND. We do not hand-list +// the fields, since that would rot the moment cpu_features adds one; instead we walk every enum +// value via cpu_features' own enum-value reader and compare it against the field-wise AND. If +// cpu_features ever adds a non-bitfield member, the byte-wise AND diverges from this field-wise +// truth and the test below fails. +#ifdef CPU_FEATURES_ARCH_X86_64 +TEST_F(SpacesTest, CpuFeatureIntersectionX86) { + auto detected = getCpuOptimizationFeatures(); + + // An all-detected mask ANDed with a mask that clears one feature yields that feature cleared. + auto clears_one = detected; + clears_one.sse = 0; + auto cleared_result = intersectWithDetectedFeatures(clears_one, detected); + ASSERT_EQ(cpu_features::GetX86FeaturesEnumValue(&cleared_result, cpu_features::X86_SSE), 0); + + // This is the hole being closed: build a detected set that (synthetically) lacks a feature, + // and an override that claims it anyway. The intersection must keep it clear regardless. + auto lacks_avx512f = detected; + lacks_avx512f.avx512f = 0; + auto claims_avx512f = lacks_avx512f; + claims_avx512f.avx512f = 1; + auto claimed_result = intersectWithDetectedFeatures(claims_avx512f, lacks_avx512f); + ASSERT_EQ(cpu_features::GetX86FeaturesEnumValue(&claimed_result, cpu_features::X86_AVX512F), 0); + + // General correctness for both scenarios above: every feature in the intersection must equal + // the logical AND of the two masks it was built from. + // GetX86FeaturesEnumValue returns the raw `int : 1;` field, which for a set signed + // 1-bit field reads back as -1 (sign-extended), not 1. Compare as bool so the sign + // representation of "set" does not matter, only whether the bit is set. + for (int i = 0; i < cpu_features::X86_LAST_; i++) { + auto value = static_cast(i); + bool expected_cleared = cpu_features::GetX86FeaturesEnumValue(&clears_one, value) && + cpu_features::GetX86FeaturesEnumValue(&detected, value); + ASSERT_EQ(static_cast(cpu_features::GetX86FeaturesEnumValue(&cleared_result, value)), + expected_cleared) + << "feature " << cpu_features::GetX86FeaturesEnumName(value); + + bool expected_claimed = cpu_features::GetX86FeaturesEnumValue(&claims_avx512f, value) && + cpu_features::GetX86FeaturesEnumValue(&lacks_avx512f, value); + ASSERT_EQ(static_cast(cpu_features::GetX86FeaturesEnumValue(&claimed_result, value)), + expected_claimed) + << "feature " << cpu_features::GetX86FeaturesEnumName(value); + } +} +#endif // CPU_FEATURES_ARCH_X86_64 + +#ifdef CPU_FEATURES_ARCH_AARCH64 +TEST_F(SpacesTest, CpuFeatureIntersectionAarch64) { + auto detected = getCpuOptimizationFeatures(); + + // An all-detected mask ANDed with a mask that clears one feature yields that feature cleared. + auto clears_one = detected; + clears_one.asimd = 0; + auto cleared_result = intersectWithDetectedFeatures(clears_one, detected); + ASSERT_EQ( + cpu_features::GetAarch64FeaturesEnumValue(&cleared_result, cpu_features::AARCH64_ASIMD), 0); + + // This is the hole being closed: build a detected set that (synthetically) lacks a feature, + // and an override that claims it anyway. The intersection must keep it clear regardless. + auto lacks_sve = detected; + lacks_sve.sve = 0; + auto claims_sve = lacks_sve; + claims_sve.sve = 1; + auto claimed_result = intersectWithDetectedFeatures(claims_sve, lacks_sve); + ASSERT_EQ(cpu_features::GetAarch64FeaturesEnumValue(&claimed_result, cpu_features::AARCH64_SVE), + 0); + + // General correctness for both scenarios above: every feature in the intersection must equal + // the logical AND of the two masks it was built from. + // GetAarch64FeaturesEnumValue returns the raw `int : 1;` field, which for a set signed + // 1-bit field reads back as -1 (sign-extended), not 1. Compare as bool so the sign + // representation of "set" does not matter, only whether the bit is set. + for (int i = 0; i < cpu_features::AARCH64_LAST_; i++) { + auto value = static_cast(i); + bool expected_cleared = cpu_features::GetAarch64FeaturesEnumValue(&clears_one, value) && + cpu_features::GetAarch64FeaturesEnumValue(&detected, value); + ASSERT_EQ( + static_cast(cpu_features::GetAarch64FeaturesEnumValue(&cleared_result, value)), + expected_cleared) + << "feature " << cpu_features::GetAarch64FeaturesEnumName(value); + + bool expected_claimed = cpu_features::GetAarch64FeaturesEnumValue(&claims_sve, value) && + cpu_features::GetAarch64FeaturesEnumValue(&lacks_sve, value); + ASSERT_EQ( + static_cast(cpu_features::GetAarch64FeaturesEnumValue(&claimed_result, value)), + expected_claimed) + << "feature " << cpu_features::GetAarch64FeaturesEnumName(value); + } +} +#endif // CPU_FEATURES_ARCH_AARCH64 From d7ac9f0c70912e93bb5209c8d7db9a3b7426b0e4 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 15:17:54 +0300 Subject: [PATCH 06/14] Enforce the conservative architecture baseline for the scalar fallback VectorSimilaritySpaces_no_optimization (L2/L2.cpp, IP/IP.cpp) is the scalar fallback that runtime dispatch degrades to, and the "no architecture flag" property that makes it safe on every deployment CPU was only a comment. The root CMakeLists.txt repeatedly appends to whatever CMAKE_CXX_FLAGS it inherits, and consumers export CXXFLAGS into this build (RediSearch's build.sh does), so an inherited -march=native could silently vectorize the fallback itself with instructions the target CPU may not have, defeating runtime dispatch in the one place that must be universally safe. Pin a conservative baseline (-march=x86-64 or -march=armv8-a, guarded by CHECK_CXX_COMPILER_FLAG, empty on other architectures) and apply it with target_compile_options(... PRIVATE ...) to VectorSimilaritySpaces_no_optimization and VectorSimilaritySpaces. The target-level flag cannot clobber the per-tier ISA flags that set_source_files_properties() attaches to each functions/*.cpp, because CMake emits source-level COMPILE_FLAGS after target-level COMPILE_OPTIONS on the same command line, and gcc does not let a later -march= disable an earlier -m feature flag either way. Reject at configure time any -march=native, -mtune=native, -mcpu=native, or -march=/-mcpu= set to something other than the chosen baseline, found in CMAKE_CXX_FLAGS, CMAKE_CXX_FLAGS_, or the CXXFLAGS environment variable. A non-native -mtune value is deliberately still allowed: it only changes instruction scheduling, not which instructions are legal, so rejecting it would break legitimate consumer builds for no safety gain. Add a small, separate CI job that injects -march=native through CXXFLAGS and asserts the configure fails; it proves a negative, so it stays out of the main build jobs. The job is dispatched through event-pull_request.yml like the other build jobs, instead of triggering on pull_request directly. Its job id is listed in pr-validation's needs, so a failure there blocks the pull request like every other required check. --- .github/workflows/event-pull_request.yml | 5 ++ .github/workflows/spaces-isa-baseline.yml | 35 ++++++++++ src/VecSim/spaces/CMakeLists.txt | 84 +++++++++++++++++++++++ 3 files changed, 124 insertions(+) create mode 100644 .github/workflows/spaces-isa-baseline.yml diff --git a/.github/workflows/event-pull_request.yml b/.github/workflows/event-pull_request.yml index 1dba65a53..af1be19a6 100644 --- a/.github/workflows/event-pull_request.yml +++ b/.github/workflows/event-pull_request.yml @@ -70,6 +70,10 @@ jobs: uses: ./.github/workflows/codeql-analysis.yml secrets: inherit + spaces-isa-baseline: + uses: ./.github/workflows/spaces-isa-baseline.yml + secrets: inherit + spellcheck: runs-on: ubuntu-latest steps: @@ -88,6 +92,7 @@ jobs: - sanitizer - coverage - codeql-analysis + - spaces-isa-baseline - spellcheck runs-on: ubuntu-latest if: ${{ !cancelled() }} diff --git a/.github/workflows/spaces-isa-baseline.yml b/.github/workflows/spaces-isa-baseline.yml new file mode 100644 index 000000000..d3d707e06 --- /dev/null +++ b/.github/workflows/spaces-isa-baseline.yml @@ -0,0 +1,35 @@ +name: spaces ISA baseline guard + +# This job asserts a negative: that configuring with an ISA-raising flag injected through +# CXXFLAGS makes CMake fail, because src/VecSim/spaces/CMakeLists.txt rejects it. It is kept +# separate from the main build jobs, since a green run here proves nothing about the build +# itself, only that the rejection still fires. + +on: [workflow_call, workflow_dispatch] + +jobs: + reject-native-baseline: + name: configure with CXXFLAGS=-march=native must fail + runs-on: ubuntu-latest + steps: + - name: checkout + uses: actions/checkout@v6 + with: + submodules: recursive + - name: assert configure fails on an inherited -march=native + env: + CXXFLAGS: -march=native + run: | + set +e + output=$(cmake -S . -B build-reject -DVECSIM_BUILD_TESTS=OFF 2>&1) + status=$? + echo "$output" + set -e + if [ "$status" -eq 0 ]; then + echo "Expected the configure to fail because of the inherited -march=native, but it succeeded." + exit 1 + fi + if ! echo "$output" | grep -q -- "-march=native"; then + echo "Configure failed, but not with the expected message about -march=native." + exit 1 + fi diff --git a/src/VecSim/spaces/CMakeLists.txt b/src/VecSim/spaces/CMakeLists.txt index eb8a15fdd..375973b31 100644 --- a/src/VecSim/spaces/CMakeLists.txt +++ b/src/VecSim/spaces/CMakeLists.txt @@ -13,6 +13,79 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror -Wall") set(OPTIMIZATIONS "") +# -------------------------------------------------------------------------------------------- +# Conservative architecture baseline for the scalar fallback. +# +# VectorSimilaritySpaces_no_optimization (L2/L2.cpp, IP/IP.cpp) is the scalar fallback that +# runtime dispatch degrades to when the CPU lacks a tier's required features, and the only +# implementation that must run on every deployment CPU. The root CMakeLists.txt repeatedly +# appends to whatever CMAKE_CXX_FLAGS it inherits, and consumers export CXXFLAGS into this +# build (RediSearch's build.sh does), so an inherited -march=native would silently let the +# compiler auto-vectorize the fallback itself with instructions the deployment CPU may not +# have, defeating runtime dispatch in the one place that must be universally safe. Pin a +# conservative baseline per target architecture below, and refuse to configure if something +# upstream already raised it. +include(CheckCXXCompilerFlag) + +set(SPACES_BASELINE_ARCH_FLAG "") +set(SPACES_BASELINE_VALUE "") +if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(AMD64|amd64)|(^i.86$)") + CHECK_CXX_COMPILER_FLAG(-march=x86-64 CXX_BASELINE_X86_64) + if(CXX_BASELINE_X86_64) + set(SPACES_BASELINE_VALUE "x86-64") + set(SPACES_BASELINE_ARCH_FLAG "-march=${SPACES_BASELINE_VALUE}") + endif() +elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv.*)") + CHECK_CXX_COMPILER_FLAG(-march=armv8-a CXX_BASELINE_ARMV8A) + if(CXX_BASELINE_ARMV8A) + set(SPACES_BASELINE_VALUE "armv8-a") + set(SPACES_BASELINE_ARCH_FLAG "-march=${SPACES_BASELINE_VALUE}") + endif() +endif() +# Any other architecture keeps an empty baseline: adding nothing is safer than guessing a flag +# the compiler may reject. A compiler that rejects the baseline flag above also degrades to +# adding nothing rather than failing the configure. + +# Reject flags that would raise that baseline outside this file's control. This library is +# built on one machine and run on another, so a native baseline (auto-detected from the build +# machine) or an explicit -march/-mcpu different from the one chosen above would let the +# compiler vectorize the scalar fallback with instructions the deployment CPU may lack, +# defeating runtime dispatch. -mtune is deliberately not checked: it only changes instruction +# scheduling, not which instructions are legal, so a non-native -mtune does not threaten the +# invariant and rejecting it would break legitimate consumer builds. +function(_spaces_reject_isa_override description flags_string) + if("${flags_string}" STREQUAL "") + return() + endif() + separate_arguments(_flags UNIX_COMMAND "${flags_string}") + foreach(_flag ${_flags}) + if(_flag MATCHES "^-m(arch|tune|cpu)=native$") + message(FATAL_ERROR + "Found '${_flag}' in ${description}. Refusing to configure: this library is " + "built on one machine and run on another, so a native ISA baseline would let " + "the compiler vectorize the scalar fallback (VectorSimilaritySpaces_no_optimization) " + "with instructions the deployment CPU may lack, defeating runtime dispatch. " + "Remove '${_flag}' from ${description}.") + elseif(_flag MATCHES "^-m(arch|cpu)=(.+)$") + if(NOT SPACES_BASELINE_VALUE STREQUAL "" AND NOT CMAKE_MATCH_2 STREQUAL SPACES_BASELINE_VALUE) + message(FATAL_ERROR + "Found '${_flag}' in ${description}. Refusing to configure: this raises the " + "ISA baseline above '${SPACES_BASELINE_ARCH_FLAG}'. This library is built on " + "one machine and run on another, so raising the baseline for the scalar " + "fallback (VectorSimilaritySpaces_no_optimization) would defeat runtime " + "dispatch. Remove '${_flag}' from ${description} or match the baseline.") + endif() + endif() + endforeach() +endfunction() + +_spaces_reject_isa_override("CMAKE_CXX_FLAGS" "${CMAKE_CXX_FLAGS}") +if(CMAKE_BUILD_TYPE) + string(TOUPPER "${CMAKE_BUILD_TYPE}" _spaces_build_type_upper) + _spaces_reject_isa_override("CMAKE_CXX_FLAGS_${_spaces_build_type_upper}" "${CMAKE_CXX_FLAGS_${_spaces_build_type_upper}}") +endif() +_spaces_reject_isa_override("the CXXFLAGS environment variable" "$ENV{CXXFLAGS}") + if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(AMD64|amd64)|(^i.86$)") # Check that the compiler supports instructions flag. # from gcc14+ -mavx512bw is implicitly enabled when -mavx512vbmi2 is requested @@ -191,3 +264,14 @@ add_library(VectorSimilaritySpaces ) target_link_libraries(VectorSimilaritySpaces VectorSimilaritySpaces_no_optimization cpu_features) + +if(NOT SPACES_BASELINE_ARCH_FLAG STREQUAL "") + # Appended at target level (PRIVATE, so it does not propagate to consumers linking this + # library) rather than folded into CMAKE_CXX_FLAGS, so it overrides anything a parent build + # inherited through CMAKE_CXX_FLAGS. CMake emits target-level COMPILE_OPTIONS before + # source-level COMPILE_FLAGS on the same command line, so the per-tier ISA flags attached + # above via set_source_files_properties() still win for the functions/*.cpp TUs that carry + # them. + target_compile_options(VectorSimilaritySpaces_no_optimization PRIVATE ${SPACES_BASELINE_ARCH_FLAG}) + target_compile_options(VectorSimilaritySpaces PRIVATE ${SPACES_BASELINE_ARCH_FLAG}) +endif() From fa9373147f157bffe79328355ace2851bc453303 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Wed, 19 Aug 2026 18:06:38 +0300 Subject: [PATCH 07/14] Address review findings on the baseline guard and mask intersection Downgrade the ISA value-mismatch case in _spaces_reject_isa_override (src/VecSim/spaces/CMakeLists.txt) from FATAL_ERROR to a warning. The target-level baseline applied further down with target_compile_options() already keeps the scalar fallback (VectorSimilaritySpaces_no_optimization) on the conservative baseline regardless of what a consumer sets, so the fatal error was never the actual protection mechanism. Refusing to configure on a merely different, explicit, non-native -march/-mcpu (for example -march=x86-64-v2, a distro default, or -mcpu=neoverse-n1) broke consumers declaring a legitimate deployment floor, and RediSearch exports CXXFLAGS into this build. The =native case keeps FATAL_ERROR: there is no safe way to honor an auto-detected baseline. De-duplicate the CMAKE_SYSTEM_PROCESSOR regex matching into a single SPACES_ARCH_FAMILY variable, computed once, with the baseline computation and both tier-gating if() blocks branching on it instead of re-matching the regex. Fix the spaces-isa-baseline workflow to grep for "Refusing to configure" instead of "-march=native": the job asserts a negative (configure must fail), and grepping for the same string it injected as CXXFLAGS would let an unrelated configure failure that happens to echo the flag pass as if it were the expected rejection. The retained FATAL_ERROR message still contains that exact phrase. Add GetCpuOptimizationFeaturesAppliesMaskX86 and GetCpuOptimizationFeaturesAppliesMaskAarch64 to tests/unit/test_spaces.cpp. The existing CpuFeatureIntersection* tests only exercise intersectWithDetectedFeatures() directly; they never call getCpuOptimizationFeatures(&mask), which is the actual entry point every real caller goes through and the function whose behavior the mask-intersection fix changed. The new tests build an all-0xFF claiming mask, deliberately not derived from the detected set, so they cannot pass vacuously. --- .github/workflows/spaces-isa-baseline.yml | 2 +- src/VecSim/spaces/CMakeLists.txt | 50 +++++++++++++++-------- tests/unit/test_spaces.cpp | 46 +++++++++++++++++++++ 3 files changed, 80 insertions(+), 18 deletions(-) diff --git a/.github/workflows/spaces-isa-baseline.yml b/.github/workflows/spaces-isa-baseline.yml index d3d707e06..bf27c9aac 100644 --- a/.github/workflows/spaces-isa-baseline.yml +++ b/.github/workflows/spaces-isa-baseline.yml @@ -29,7 +29,7 @@ jobs: echo "Expected the configure to fail because of the inherited -march=native, but it succeeded." exit 1 fi - if ! echo "$output" | grep -q -- "-march=native"; then + if ! echo "$output" | grep -q -- "Refusing to configure"; then echo "Configure failed, but not with the expected message about -march=native." exit 1 fi diff --git a/src/VecSim/spaces/CMakeLists.txt b/src/VecSim/spaces/CMakeLists.txt index 375973b31..914816599 100644 --- a/src/VecSim/spaces/CMakeLists.txt +++ b/src/VecSim/spaces/CMakeLists.txt @@ -27,15 +27,26 @@ set(OPTIMIZATIONS "") # upstream already raised it. include(CheckCXXCompilerFlag) +# SPACES_ARCH_FAMILY is computed once here from CMAKE_SYSTEM_PROCESSOR so the two arch regexes +# below are not repeated again at the tier-gating if() blocks further down (x86_64 tiers and ARM +# tiers each used to re-run the same MATCHES check). Everything downstream branches on this +# variable instead of re-matching CMAKE_SYSTEM_PROCESSOR. +set(SPACES_ARCH_FAMILY "") +if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(AMD64|amd64)|(^i.86$)") + set(SPACES_ARCH_FAMILY "x86_64") +elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv.*)") + set(SPACES_ARCH_FAMILY "aarch64") +endif() + set(SPACES_BASELINE_ARCH_FLAG "") set(SPACES_BASELINE_VALUE "") -if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(AMD64|amd64)|(^i.86$)") +if(SPACES_ARCH_FAMILY STREQUAL "x86_64") CHECK_CXX_COMPILER_FLAG(-march=x86-64 CXX_BASELINE_X86_64) if(CXX_BASELINE_X86_64) set(SPACES_BASELINE_VALUE "x86-64") set(SPACES_BASELINE_ARCH_FLAG "-march=${SPACES_BASELINE_VALUE}") endif() -elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv.*)") +elseif(SPACES_ARCH_FAMILY STREQUAL "aarch64") CHECK_CXX_COMPILER_FLAG(-march=armv8-a CXX_BASELINE_ARMV8A) if(CXX_BASELINE_ARMV8A) set(SPACES_BASELINE_VALUE "armv8-a") @@ -46,13 +57,18 @@ endif() # the compiler may reject. A compiler that rejects the baseline flag above also degrades to # adding nothing rather than failing the configure. -# Reject flags that would raise that baseline outside this file's control. This library is -# built on one machine and run on another, so a native baseline (auto-detected from the build -# machine) or an explicit -march/-mcpu different from the one chosen above would let the -# compiler vectorize the scalar fallback with instructions the deployment CPU may lack, -# defeating runtime dispatch. -mtune is deliberately not checked: it only changes instruction -# scheduling, not which instructions are legal, so a non-native -mtune does not threaten the -# invariant and rejecting it would break legitimate consumer builds. +# A native baseline (auto-detected from the build machine, so its instruction set is unknown +# ahead of time) is rejected outright below: it would let the compiler vectorize the scalar +# fallback with instructions the deployment CPU may lack, defeating runtime dispatch, and there +# is no safe way to honor it. An explicit non-native -march/-mcpu that merely differs from the +# baseline chosen above is a deliberate, known deployment floor (for example -march=x86-64-v2, +# now a distro default, or -mcpu=neoverse-n1), so it only gets a warning: the target-level +# baseline applied further down with target_compile_options() still keeps the scalar fallback +# on the conservative baseline regardless, and the per-tier source-level flags still win for the +# functions/*.cpp tiers, so the value is respected everywhere else it is meant to apply. +# -mtune is deliberately not checked: it only changes instruction scheduling, not which +# instructions are legal, so a non-native -mtune does not threaten the invariant and rejecting +# it would break legitimate consumer builds. function(_spaces_reject_isa_override description flags_string) if("${flags_string}" STREQUAL "") return() @@ -68,12 +84,12 @@ function(_spaces_reject_isa_override description flags_string) "Remove '${_flag}' from ${description}.") elseif(_flag MATCHES "^-m(arch|cpu)=(.+)$") if(NOT SPACES_BASELINE_VALUE STREQUAL "" AND NOT CMAKE_MATCH_2 STREQUAL SPACES_BASELINE_VALUE) - message(FATAL_ERROR - "Found '${_flag}' in ${description}. Refusing to configure: this raises the " - "ISA baseline above '${SPACES_BASELINE_ARCH_FLAG}'. This library is built on " - "one machine and run on another, so raising the baseline for the scalar " - "fallback (VectorSimilaritySpaces_no_optimization) would defeat runtime " - "dispatch. Remove '${_flag}' from ${description} or match the baseline.") + message(WARNING + "Found '${_flag}' in ${description}, which differs from the baseline " + "'${SPACES_BASELINE_ARCH_FLAG}' this build would otherwise pick. The conservative " + "baseline is still applied to the scalar fallback " + "(VectorSimilaritySpaces_no_optimization), so it stays safe; '${_flag}' is " + "respected everywhere else.") endif() endif() endforeach() @@ -86,7 +102,7 @@ if(CMAKE_BUILD_TYPE) endif() _spaces_reject_isa_override("the CXXFLAGS environment variable" "$ENV{CXXFLAGS}") -if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(AMD64|amd64)|(^i.86$)") +if(SPACES_ARCH_FAMILY STREQUAL "x86_64") # Check that the compiler supports instructions flag. # from gcc14+ -mavx512bw is implicitly enabled when -mavx512vbmi2 is requested include(${root}/cmake/x86_64InstructionFlags.cmake) @@ -196,7 +212,7 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64)|(AMD64|amd64)|(^i.86$)") endif() endif() -if (CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)|(ARM64)|(armv.*)") +if (SPACES_ARCH_FAMILY STREQUAL "aarch64") include(${root}/cmake/aarch64InstructionFlags.cmake) # Create different optimization implementations for ARM architecture diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 2b2fe7c7f..49a110e10 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -5009,3 +5009,49 @@ TEST_F(SpacesTest, CpuFeatureIntersectionAarch64) { } } #endif // CPU_FEATURES_ARCH_AARCH64 + +// The CpuFeatureIntersection* tests above call intersectWithDetectedFeatures() directly; they do +// not exercise getCpuOptimizationFeatures(&mask), which is the actual entry point every real +// caller (and every chooser in spaces.cpp) goes through, and the function whose behavior this +// series of fixes changed. Cover that entry point here: a mask claiming every feature still +// cannot make the result report a feature the live hardware does not have, since +// getCpuOptimizationFeatures() always intersects the passed-in mask with a fresh read of the +// real detected features, not with whatever the caller believes is detected. +#ifdef CPU_FEATURES_ARCH_X86_64 +TEST_F(SpacesTest, GetCpuOptimizationFeaturesAppliesMaskX86) { + auto detected = getCpuOptimizationFeatures(); + + auto all_claimed = detected; + std::memset(&all_claimed, 0xFF, sizeof(all_claimed)); + + auto result = getCpuOptimizationFeatures(&all_claimed); + for (int i = 0; i < cpu_features::X86_LAST_; i++) { + auto value = static_cast(i); + bool detected_bit = + static_cast(cpu_features::GetX86FeaturesEnumValue(&detected, value)); + bool result_bit = static_cast(cpu_features::GetX86FeaturesEnumValue(&result, value)); + ASSERT_EQ(result_bit, detected_bit) + << "feature " << cpu_features::GetX86FeaturesEnumName(value); + } +} +#endif // CPU_FEATURES_ARCH_X86_64 + +#ifdef CPU_FEATURES_ARCH_AARCH64 +TEST_F(SpacesTest, GetCpuOptimizationFeaturesAppliesMaskAarch64) { + auto detected = getCpuOptimizationFeatures(); + + auto all_claimed = detected; + std::memset(&all_claimed, 0xFF, sizeof(all_claimed)); + + auto result = getCpuOptimizationFeatures(&all_claimed); + for (int i = 0; i < cpu_features::AARCH64_LAST_; i++) { + auto value = static_cast(i); + bool detected_bit = + static_cast(cpu_features::GetAarch64FeaturesEnumValue(&detected, value)); + bool result_bit = + static_cast(cpu_features::GetAarch64FeaturesEnumValue(&result, value)); + ASSERT_EQ(result_bit, detected_bit) + << "feature " << cpu_features::GetAarch64FeaturesEnumName(value); + } +} +#endif // CPU_FEATURES_ARCH_AARCH64 From 8e7142147768fabbcedfcd785657b6e995a56476 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Thu, 20 Aug 2026 14:10:48 +0300 Subject: [PATCH 08/14] Fail closed on the baseline probe and refuse explicit ISA feature flags Two ways the scalar fallback could still end up compiled above the deployment baseline, both found in review and both measured on hardware. The baseline probe failed open. CHECK_CXX_COMPILER_FLAG compiles its test with CMAKE_CXX_FLAGS included and treats a warning that names a flag as failure, so an inherited -mcpu=neoverse-n1 makes gcc warn that the switch conflicts with -march=armv8-a and the probe reports Failed. On x86_64 and aarch64 the baseline flag is supported by every compiler that can build this library, so a failed probe never means the flag is unsupported: it means the inherited flags broke the probe. Continuing there dropped the baseline from the scalar fallback while the configure still succeeded, which is the hazard the guard exists to prevent. It now fails closed and names the likely cause. Verified on a Neoverse-N1. Explicit ISA feature flags bypassed the guard entirely. It only matched -march and -mcpu, and a later -march= baseline does not cancel a feature flag: measured with gcc 13, "-mavx2 ... -march=x86-64" still emitted 317 and 161 AVX-family instructions into the two scalar fallback objects. Those flags cannot be neutralised reliably, so they are refused. Disabling forms (-mno-...) and -mtune= stay allowed, since neither raises the set of legal instructions. The guard's CI job asserted only the -march=native case, so it caught neither of these. It is now a matrix over the native variants and the feature flags, plus a positive case checking that an explicit non-native baseline still configures and still warns. Also gate the 22 single-line SQ8 VNNI test guards on avx512vl, so the tests mirror the production predicate this branch tightened. The multi-line INT8 and UINT8 guards already required it. --- .github/workflows/spaces-isa-baseline.yml | 62 +++++++++++++++++---- src/VecSim/spaces/CMakeLists.txt | 38 ++++++++++++- tests/unit/test_spaces.cpp | 66 +++++++++++++++-------- 3 files changed, 132 insertions(+), 34 deletions(-) diff --git a/.github/workflows/spaces-isa-baseline.yml b/.github/workflows/spaces-isa-baseline.yml index bf27c9aac..7527c72e4 100644 --- a/.github/workflows/spaces-isa-baseline.yml +++ b/.github/workflows/spaces-isa-baseline.yml @@ -1,24 +1,40 @@ name: spaces ISA baseline guard -# This job asserts a negative: that configuring with an ISA-raising flag injected through -# CXXFLAGS makes CMake fail, because src/VecSim/spaces/CMakeLists.txt rejects it. It is kept -# separate from the main build jobs, since a green run here proves nothing about the build -# itself, only that the rejection still fires. +# These jobs assert negatives: that configuring with an ISA-raising flag injected through CXXFLAGS +# makes CMake fail, because src/VecSim/spaces/CMakeLists.txt rejects it. They are kept separate +# from the main build jobs, since a green run here proves nothing about the build itself, only that +# each rejection still fires. Every case below was a real bypass at some point in review, so a case +# that stops failing is a regression, not a cleanup opportunity. on: [workflow_call, workflow_dispatch] jobs: - reject-native-baseline: - name: configure with CXXFLAGS=-march=native must fail + reject-isa-override: + name: "configure must fail: CXXFLAGS=${{ matrix.flag }}" runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + # A native baseline is unknown at build time, so it can raise the fallback's ISA above + # any deployment CPU. + - flag: -march=native + - flag: -mtune=native + - flag: -mcpu=native + # An explicit feature flag is NOT cancelled by a later -march= baseline: measured on + # gcc 13, "-mavx2 ... -march=x86-64" still emitted hundreds of AVX instructions into the + # scalar fallback. + - flag: -mavx2 + - flag: -mavx512f + - flag: -mfma steps: - name: checkout uses: actions/checkout@v6 with: submodules: recursive - - name: assert configure fails on an inherited -march=native + - name: assert configure fails on an inherited ${{ matrix.flag }} env: - CXXFLAGS: -march=native + CXXFLAGS: ${{ matrix.flag }} run: | set +e output=$(cmake -S . -B build-reject -DVECSIM_BUILD_TESTS=OFF 2>&1) @@ -26,10 +42,36 @@ jobs: echo "$output" set -e if [ "$status" -eq 0 ]; then - echo "Expected the configure to fail because of the inherited -march=native, but it succeeded." + echo "Expected the configure to fail because of the inherited ${{ matrix.flag }}, but it succeeded." exit 1 fi if ! echo "$output" | grep -q -- "Refusing to configure"; then - echo "Configure failed, but not with the expected message about -march=native." + echo "Configure failed, but not with the expected rejection message." + exit 1 + fi + + accept-explicit-baseline: + name: "configure must succeed and warn: CXXFLAGS=-march=x86-64-v2" + runs-on: ubuntu-latest + steps: + - name: checkout + uses: actions/checkout@v6 + with: + submodules: recursive + - name: assert an explicit non-native baseline still configures + env: + CXXFLAGS: -march=x86-64-v2 + run: | + set +e + output=$(cmake -S . -B build-accept -DVECSIM_BUILD_TESTS=OFF 2>&1) + status=$? + echo "$output" + set -e + if [ "$status" -ne 0 ]; then + echo "A consumer declaring a legitimate deployment floor must still be able to build." + exit 1 + fi + if ! echo "$output" | grep -qi "baseline"; then + echo "Expected a warning naming the baseline, so the difference is not silent." exit 1 fi diff --git a/src/VecSim/spaces/CMakeLists.txt b/src/VecSim/spaces/CMakeLists.txt index 914816599..76fcdeee2 100644 --- a/src/VecSim/spaces/CMakeLists.txt +++ b/src/VecSim/spaces/CMakeLists.txt @@ -54,8 +54,30 @@ elseif(SPACES_ARCH_FAMILY STREQUAL "aarch64") endif() endif() # Any other architecture keeps an empty baseline: adding nothing is safer than guessing a flag -# the compiler may reject. A compiler that rejects the baseline flag above also degrades to -# adding nothing rather than failing the configure. +# the compiler may reject. +# +# On x86_64 and aarch64, though, the baseline flag probed above is supported by every gcc and clang +# that can build this library, so a failed probe never means "this compiler lacks the flag". It +# means something in the inherited flags made the probe fail. CHECK_CXX_COMPILER_FLAG compiles its +# test with CMAKE_CXX_FLAGS included and treats any warning that names a flag as failure, so an +# inherited -mcpu=neoverse-n1 makes gcc emit +# switch '-mcpu=neoverse-n1' conflicts with '-march=armv8-a' switch +# and the probe reports Failed. Failing open there would silently drop the baseline from the scalar +# fallback while the configure still succeeded, which is the exact hazard this block exists to +# prevent, so fail closed instead and name the likely cause. +if(SPACES_ARCH_FAMILY STREQUAL "x86_64" OR SPACES_ARCH_FAMILY STREQUAL "aarch64") + if(SPACES_BASELINE_ARCH_FLAG STREQUAL "") + message(FATAL_ERROR + "Could not establish a conservative ISA baseline on ${SPACES_ARCH_FAMILY}: the probe " + "for the baseline -march flag failed. Refusing to configure, because continuing would " + "leave the scalar fallback (VectorSimilaritySpaces_no_optimization) compiled with " + "whatever ISA the inherited flags imply, defeating runtime dispatch on any deployment " + "CPU that lacks it. The usual cause is an inherited -mcpu= or -march= in CMAKE_CXX_FLAGS " + "or CXXFLAGS that conflicts with the baseline flag: the probe compiles with those flags " + "present and counts a conflict warning as failure. Current CMAKE_CXX_FLAGS: " + "'${CMAKE_CXX_FLAGS}'.") + endif() +endif() # A native baseline (auto-detected from the build machine, so its instruction set is unknown # ahead of time) is rejected outright below: it would let the compiler vectorize the scalar @@ -82,6 +104,18 @@ function(_spaces_reject_isa_override description flags_string) "the compiler vectorize the scalar fallback (VectorSimilaritySpaces_no_optimization) " "with instructions the deployment CPU may lack, defeating runtime dispatch. " "Remove '${_flag}' from ${description}.") + elseif(_flag MATCHES "^-m(mmx|3dnow[a-z]*|sse[0-9a-z_.]*|avx[0-9a-z_.]*|fma[0-9a-z_.]*|f16c|bmi[0-9]*|abm|lzcnt|popcnt|aes|vaes|sha|pclmul|gfni|xop|amx[-a-z0-9_]*|fp16|dotprod|crypto|simd)$") + message(FATAL_ERROR + "Found '${_flag}' in ${description}. Refusing to configure: an explicit ISA " + "feature flag enables those instructions directly and a later -march= baseline " + "does NOT cancel it. Measured on gcc 13: '-mavx2 ... -march=x86-64' still emitted " + "hundreds of AVX instructions into the scalar fallback " + "(VectorSimilaritySpaces_no_optimization), which is the one implementation runtime " + "dispatch degrades to when the CPU lacks everything else, so the fallback itself " + "would fault on a deployment CPU without ${_flag}'s instructions. There is no " + "reliable way to neutralize it, so remove '${_flag}' from ${description}. Disabling " + "forms (-mno-...) are not affected, and -mtune= is fine: it changes scheduling, not " + "which instructions are legal.") elseif(_flag MATCHES "^-m(arch|cpu)=(.+)$") if(NOT SPACES_BASELINE_VALUE STREQUAL "" AND NOT CMAKE_MATCH_2 STREQUAL SPACES_BASELINE_VALUE) message(WARNING diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 49a110e10..1175fd324 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -2356,7 +2356,8 @@ TEST_P(SQ8_FP32_SpacesOptimizationTest, SQ8_FP32_L2SqrTest) { float baseline = SQ8_FP32_L2Sqr(v2_compressed.data(), v1_orig.data(), dim); // Test different optimizations based on CPU features #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; arch_opt_func = L2_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_FP32_L2_implementation_AVX512F_BW_VL_VNNI(dim)) @@ -2482,7 +2483,8 @@ TEST_P(SQ8_FP32_SpacesOptimizationTest, SQ8_FP32_InnerProductTest) { // Test different optimizations based on CPU features #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; arch_opt_func = IP_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_FP32_IP_implementation_AVX512F_BW_VL_VNNI(dim)) @@ -2633,7 +2635,8 @@ TEST_P(SQ8_FP32_SpacesOptimizationTest, SQ8_FP32_CosineTest) { // Test different optimizations based on CPU features #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_FP32_Cosine_implementation_AVX512F_BW_VL_VNNI(dim)) @@ -2745,7 +2748,8 @@ TEST(SQ8_FP32_EdgeCases, SelfDistanceCosine) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); @@ -2846,7 +2850,8 @@ TEST(SQ8_FP32_EdgeCases, SelfDistanceL2) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_quantized.data(), v_orig.data(), dim); @@ -2955,7 +2960,8 @@ TEST(SQ8_FP32_EdgeCases, CosineSymmetryTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); float cos_12 = arch_opt_func(v2_quantized.data(), v1_fp32.data(), dim); @@ -3022,7 +3028,8 @@ TEST(SQ8_FP32_EdgeCases, CosineZeroVectorTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_nonzero_quantized.data(), v_zero.data(), dim); @@ -3102,7 +3109,8 @@ TEST(SQ8_FP32_EdgeCases, CosineConstantVectorTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_const_quantized.data(), v_query.data(), dim); @@ -3182,7 +3190,8 @@ TEST(SQ8_FP32_EdgeCases, CosineExtremeValuesTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_FP32_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v2_quantized.data(), v1.data(), dim); @@ -3925,7 +3934,8 @@ TEST_P(SQ8_SQ8_SpacesOptimizationTest, SQ8_SQ8_InnerProductTest) { #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; arch_opt_func = IP_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_SQ8_IP_implementation_AVX512F_BW_VL_VNNI(dim)) @@ -4008,7 +4018,8 @@ TEST_P(SQ8_SQ8_SpacesOptimizationTest, SQ8_SQ8_CosineTest) { #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; arch_opt_func = Cosine_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_SQ8_Cosine_implementation_AVX512F_BW_VL_VNNI(dim)) @@ -4091,7 +4102,8 @@ TEST_P(SQ8_SQ8_SpacesOptimizationTest, SQ8_SQ8_L2SqrTest) { #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; arch_opt_func = L2_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); ASSERT_EQ(arch_opt_func, Choose_SQ8_SQ8_L2_implementation_AVX512F_BW_VL_VNNI(dim)) @@ -4170,7 +4182,8 @@ TEST(SQ8_SQ8_EdgeCases, SelfDistanceCosine) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_quantized.data(), v_quantized.data(), dim); @@ -4242,7 +4255,8 @@ TEST(SQ8_SQ8_EdgeCases, CosineSymmetryTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float cos_12 = arch_opt_func(v1_quantized.data(), v2_quantized.data(), dim); @@ -4310,7 +4324,8 @@ TEST(SQ8_SQ8_EdgeCases, CosineZeroVectorTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_zero_quantized.data(), v_nonzero_quantized.data(), dim); @@ -4382,7 +4397,8 @@ TEST(SQ8_SQ8_EdgeCases, CosineConstantVectorTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_const_quantized.data(), v_random_quantized.data(), dim); @@ -4463,7 +4479,8 @@ TEST(SQ8_SQ8_EdgeCases, CosineExtremeValuesTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = Cosine_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v1_quantized.data(), v2_quantized.data(), dim); @@ -4532,7 +4549,8 @@ TEST(SQ8_SQ8_EdgeCases, SelfDistanceL2) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_quantized.data(), v_quantized.data(), dim); @@ -4604,7 +4622,8 @@ TEST(SQ8_SQ8_EdgeCases, L2SymmetryTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float l2_12 = arch_opt_func(v1_quantized.data(), v2_quantized.data(), dim); @@ -4672,7 +4691,8 @@ TEST(SQ8_SQ8_EdgeCases, L2ZeroVectorTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_zero_quantized.data(), v_nonzero_quantized.data(), dim); @@ -4743,7 +4763,8 @@ TEST(SQ8_SQ8_EdgeCases, L2ConstantVectorTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v_const_quantized.data(), v_random_quantized.data(), dim); @@ -4817,7 +4838,8 @@ TEST(SQ8_SQ8_EdgeCases, L2ExtremeValuesTest) { } #endif #ifdef OPT_AVX512_F_BW_VL_VNNI - if (optimization.avx512f && optimization.avx512bw && optimization.avx512vnni) { + if (optimization.avx512f && optimization.avx512bw && optimization.avx512vl && + optimization.avx512vnni) { unsigned char alignment = 0; auto arch_opt_func = L2_SQ8_SQ8_GetDistFunc(dim, &alignment, &optimization); float result = arch_opt_func(v1_quantized.data(), v2_quantized.data(), dim); From bd6526054810d30e7aa0e9c5b1d483050fc451ce Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Thu, 20 Aug 2026 17:46:57 +0300 Subject: [PATCH 09/14] Accumulate the float16 SIMD kernels in fp32 on x86 and ARM `FP16_L2Sqr` and `FP16_InnerProduct` widen each stored half with FP16_to_FP32 and accumulate into a `float`. Four SIMD tiers did not: AVX512FP16 kept `__m512h sum` and reduced into a `_Float16`, NEON kept `float16x8_t acc` with `vfmaq_f16`, and SVE kept `svfloat16_t acc` with `svmla_f16_x`. On any CPU that selects one of those tiers the whole vector was summed in an 11-bit mantissa, so the same function returned a different answer depending on the machine. Two consequences, both silent. Precision: simulating both accumulation orders over 20,000 random 128-dimension fp16 vectors gives a maximum relative error of 5.6e-3 for the fp16 accumulator against 1.9e-7 for the fp32 one, so nearest neighbours and their ordering change. Overflow: 65504 is the largest finite fp16 value, so 32 elements of 200.0, all ordinary fp16 values, drive a half precision accumulator past it and the result becomes infinity. All four tiers now widen and accumulate in fp32. The x86 kernels mirror L2_AVX512F_FP16.h and IP_AVX512F_FP16.h step for step, since after widening there is nothing half-precision-specific left to do differently; they also gain the second accumulator that #984 added to the sibling fp16 tiers and not to these. NEON and SVE keep their existing four-way unrolling, with each accumulator becoming a pair covering the lower and upper halves of a register. The unit tests could not have caught this. Both fp16 baselines accumulated the reference in `_Float16` too, so the test compared a half precision kernel against a half precision reference and passed within its 1% tolerance whatever the kernel did. They now accumulate in `float` via FP16_to_FP32, mirroring the scalar functions exactly. That alone is still not a regression test: the randomized cases draw values from [-0.99, 0.99], where a half precision accumulator's worst error over dim 32..256 is about 0.54%, inside the 1% budget. FP16SpacesTest.LargeValuesDoNotOverflowTheAccumulator closes that gap with inputs whose expected totals are exact in fp32 and infinite in fp16, and it calls the public choosers so whichever tier the running CPU selects is tested. The ARM half of this change depends on the NEON_HP/NEON_FHM translation unit split from #1018. Widening to fp32 and then issuing vfmaq_f32 is exactly the pattern gcc contracts into FMLAL, so in a translation unit compiled with +fp16fml the fix would emit FMLAL into the plain half-precision path and fault on any core without FEAT_FHM. Measured on gcc 12: the NEON kernels compile to 4 FMLAL at -march=armv8.2-a+fp16fml and 0 at +fp16. With #1018 the HP tier is compiled at +fp16 only, and NEON_HP.cpp.o contains no FMLAL. Verified on an AWS Graviton2 (Neoverse-N1, asimdhp without asimdfhm, gcc 12), which executes the NEON path: the full spaces suite passes 1529/1529 with this change on top of #1019, where the same change on a main base fails 106 tests with SIGILL. On x86 (Ice Lake, gcc 13) the suite passes 1569/1569, and the AVX512FP16 kernels compile with -mavx512fp16 -Werror leaving no fmadd*ph, subph or mulph. The AVX512FP16 tier itself needs Sapphire Rapids or later to execute and the SVE tier needs an SVE core, so neither runs on the hardware available here; both are covered by CI. --- src/VecSim/spaces/IP/IP_AVX512FP16_VL_FP16.h | 54 +++++++----- src/VecSim/spaces/IP/IP_NEON_FP16.h | 69 ++++++++-------- src/VecSim/spaces/IP/IP_SVE_FP16.h | 86 ++++++++++++++------ src/VecSim/spaces/L2/L2_AVX512FP16_VL_FP16.h | 64 +++++++++------ src/VecSim/spaces/L2/L2_NEON_FP16.h | 75 +++++++++-------- src/VecSim/spaces/L2/L2_SVE_FP16.h | 83 ++++++++++++------- tests/unit/test_spaces.cpp | 64 ++++++++++++--- 7 files changed, 318 insertions(+), 177 deletions(-) diff --git a/src/VecSim/spaces/IP/IP_AVX512FP16_VL_FP16.h b/src/VecSim/spaces/IP/IP_AVX512FP16_VL_FP16.h index 130ff2c7a..9e53092ce 100644 --- a/src/VecSim/spaces/IP/IP_AVX512FP16_VL_FP16.h +++ b/src/VecSim/spaces/IP/IP_AVX512FP16_VL_FP16.h @@ -6,6 +6,7 @@ * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the * GNU Affero General Public License v3 (AGPLv3). */ +#pragma once #include #include "VecSim/spaces/space_includes.h" #include "VecSim/types/float16.h" @@ -13,13 +14,17 @@ using float16 = vecsim_types::float16; -static void InnerProductStep(float16 *&pVect1, float16 *&pVect2, __m512h &sum) { - __m512h v1 = _mm512_loadu_ph(pVect1); - __m512h v2 = _mm512_loadu_ph(pVect2); - - sum = _mm512_fmadd_ph(v1, v2, sum); - pVect1 += 32; - pVect2 += 32; +// See the note in L2_AVX512FP16_VL_FP16.h: the products accumulate in fp32, matching +// `FP16_InnerProduct` and mirroring IP_AVX512F_FP16.h, because a half precision accumulator both +// loses precision on every add and overflows to infinity past 65504, the largest finite fp16 value. +static void InnerProductStep(float16 *&pVect1, float16 *&pVect2, __m512 &sum) { + // Convert 16 half-floats into floats and store them in 512 bits register. + auto v1 = _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect1)); + auto v2 = _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect2)); + + sum = _mm512_fmadd_ps(v1, v2, sum); + pVect1 += 16; + pVect2 += 16; } template // 0..31 @@ -30,22 +35,29 @@ float FP16_InnerProductSIMD32_AVX512FP16_VL(const void *pVect1v, const void *pVe const float16 *pEnd1 = pVect1 + dimension; - __m512h sum = _mm512_setzero_ph(); - - if constexpr (residual) { - constexpr __mmask32 mask = (1LU << residual) - 1; - __m512h v1 = _mm512_loadu_ph(pVect1); - pVect1 += residual; - __m512h v2 = _mm512_loadu_ph(pVect2); - pVect2 += residual; - sum = _mm512_maskz_mul_ph(mask, v1, v2); + // Two accumulators break the FMA dependency chain, letting more FMAs be in flight at once. + auto sum0 = _mm512_setzero_ps(); + auto sum1 = _mm512_setzero_ps(); + + if constexpr (residual % 16) { + __mmask16 constexpr residuals_mask = (1 << (residual % 16)) - 1; + auto v1 = _mm512_maskz_mov_ps(residuals_mask, + _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect1))); + auto v2 = _mm512_maskz_mov_ps(residuals_mask, + _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect2))); + sum0 = _mm512_mul_ps(v1, v2); + pVect1 += residual % 16; + pVect2 += residual % 16; + } + if constexpr (residual >= 16) { + InnerProductStep(pVect1, pVect2, sum1); } // We dealt with the residual part. We are left with some multiple of 32 16-bit floats. - do { - InnerProductStep(pVect1, pVect2, sum); - } while (pVect1 < pEnd1); + while (pVect1 < pEnd1) { + InnerProductStep(pVect1, pVect2, sum0); + InnerProductStep(pVect1, pVect2, sum1); + } - _Float16 res = _mm512_reduce_add_ph(sum); - return _Float16(1) - res; + return 1.0f - _mm512_reduce_add_ps(_mm512_add_ps(sum0, sum1)); } diff --git a/src/VecSim/spaces/IP/IP_NEON_FP16.h b/src/VecSim/spaces/IP/IP_NEON_FP16.h index fd547c457..93f9f06e0 100644 --- a/src/VecSim/spaces/IP/IP_NEON_FP16.h +++ b/src/VecSim/spaces/IP/IP_NEON_FP16.h @@ -6,17 +6,26 @@ * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the * GNU Affero General Public License v3 (AGPLv3). */ +#pragma once #include -inline void InnerProduct_Step(const float16_t *&vec1, const float16_t *&vec2, float16x8_t &acc) { +// The products accumulate in fp32, not fp16. `FP16_InnerProduct` widens each stored half with +// FP16_to_FP32 and accumulates into a float, so a half precision accumulator would make this tier +// disagree with the same function computed anywhere else. It is also unsafe: 65504 is the largest +// finite fp16 value, so 32 elements of 200.0, all ordinary fp16 values, drive a half precision +// accumulator past it and the result becomes infinity. An fp32 accumulator cannot overflow for +// any fp16 input. +inline void InnerProduct_Step(const float16_t *&vec1, const float16_t *&vec2, float32x4_t &acc_lo, + float32x4_t &acc_hi) { // Load half-precision vectors float16x8_t v1 = vld1q_f16(vec1); float16x8_t v2 = vld1q_f16(vec2); vec1 += 8; vec2 += 8; - // Multiply and accumulate - acc = vfmaq_f16(acc, v1, v2); + // Widen both halves to single precision, then multiply and accumulate in fp32. + acc_lo = vfmaq_f32(acc_lo, vcvt_f32_f16(vget_low_f16(v1)), vcvt_f32_f16(vget_low_f16(v2))); + acc_hi = vfmaq_f32(acc_hi, vcvt_high_f32_f16(v1), vcvt_high_f32_f16(v2)); } template // 0..31 @@ -24,10 +33,11 @@ float FP16_InnerProduct_NEON_HP(const void *pVect1v, const void *pVect2v, size_t const auto *vec1 = static_cast(pVect1v); const auto *vec2 = static_cast(pVect2v); const auto *const v1End = vec1 + dimension; - float16x8_t acc1 = vdupq_n_f16(0.0f); - float16x8_t acc2 = vdupq_n_f16(0.0f); - float16x8_t acc3 = vdupq_n_f16(0.0f); - float16x8_t acc4 = vdupq_n_f16(0.0f); + // Four accumulator pairs, keeping the original four-way unrolling with fp32 accumulators. + float32x4_t acc1_lo = vdupq_n_f32(0.0f), acc1_hi = vdupq_n_f32(0.0f); + float32x4_t acc2_lo = vdupq_n_f32(0.0f), acc2_hi = vdupq_n_f32(0.0f); + float32x4_t acc3_lo = vdupq_n_f32(0.0f), acc3_hi = vdupq_n_f32(0.0f); + float32x4_t acc4_lo = vdupq_n_f32(0.0f), acc4_hi = vdupq_n_f32(0.0f); // First, handle the partial chunk residual if constexpr (residual % 8) { @@ -48,12 +58,15 @@ float FP16_InnerProduct_NEON_HP(const void *pVect1v, const void *pVect2v, size_t float16x8_t v1 = vld1q_f16(vec1); float16x8_t v2 = vld1q_f16(vec2); - // Apply mask to both vectors - float16x8_t masked_v1 = vbslq_f16(mask, v1, acc1); // `acc1` should be all zeros here - float16x8_t masked_v2 = vbslq_f16(mask, v2, acc2); // `acc2` should be all zeros here + // Apply mask to both vectors, zeroing the lanes past the residual. + const float16x8_t zero_h = vdupq_n_f16(0.0f); + float16x8_t masked_v1 = vbslq_f16(mask, v1, zero_h); + float16x8_t masked_v2 = vbslq_f16(mask, v2, zero_h); - // Multiply and accumulate - acc1 = vfmaq_f16(acc1, masked_v1, masked_v2); + // Multiply and accumulate in fp32; the masked lanes contribute zero. + acc1_lo = vfmaq_f32(acc1_lo, vcvt_f32_f16(vget_low_f16(masked_v1)), + vcvt_f32_f16(vget_low_f16(masked_v2))); + acc1_hi = vfmaq_f32(acc1_hi, vcvt_high_f32_f16(masked_v1), vcvt_high_f32_f16(masked_v2)); // Advance pointers vec1 += chunk_residual; @@ -62,34 +75,26 @@ float FP16_InnerProduct_NEON_HP(const void *pVect1v, const void *pVect2v, size_t // Handle (residual - (residual % 8)) in chunks of 8 float16 if constexpr (residual >= 8) - InnerProduct_Step(vec1, vec2, acc2); + InnerProduct_Step(vec1, vec2, acc2_lo, acc2_hi); if constexpr (residual >= 16) - InnerProduct_Step(vec1, vec2, acc3); + InnerProduct_Step(vec1, vec2, acc3_lo, acc3_hi); if constexpr (residual >= 24) - InnerProduct_Step(vec1, vec2, acc4); + InnerProduct_Step(vec1, vec2, acc4_lo, acc4_hi); // Process the rest of the vectors (the full chunks part) while (vec1 < v1End) { // TODO: use `vld1q_f16_x4` for quad-loading? - InnerProduct_Step(vec1, vec2, acc1); - InnerProduct_Step(vec1, vec2, acc2); - InnerProduct_Step(vec1, vec2, acc3); - InnerProduct_Step(vec1, vec2, acc4); + InnerProduct_Step(vec1, vec2, acc1_lo, acc1_hi); + InnerProduct_Step(vec1, vec2, acc2_lo, acc2_hi); + InnerProduct_Step(vec1, vec2, acc3_lo, acc3_hi); + InnerProduct_Step(vec1, vec2, acc4_lo, acc4_hi); } - // Accumulate accumulators - acc1 = vpaddq_f16(acc1, acc3); - acc2 = vpaddq_f16(acc2, acc4); - acc1 = vpaddq_f16(acc1, acc2); - - // Horizontal sum of the accumulated values - float32x4_t sum_f32 = vcvt_f32_f16(vget_low_f16(acc1)); - sum_f32 = vaddq_f32(sum_f32, vcvt_f32_f16(vget_high_f16(acc1))); - - // Pairwise add to get horizontal sum - float32x2_t sum_2 = vadd_f32(vget_low_f32(sum_f32), vget_high_f32(sum_f32)); - sum_2 = vpadd_f32(sum_2, sum_2); + // Accumulate accumulators, all in fp32. + float32x4_t sum_f32 = vaddq_f32(vaddq_f32(acc1_lo, acc1_hi), vaddq_f32(acc2_lo, acc2_hi)); + sum_f32 = vaddq_f32(sum_f32, vaddq_f32(acc3_lo, acc3_hi)); + sum_f32 = vaddq_f32(sum_f32, vaddq_f32(acc4_lo, acc4_hi)); // Extract result - return 1.0f - vget_lane_f32(sum_2, 0); + return 1.0f - vaddvq_f32(sum_f32); } diff --git a/src/VecSim/spaces/IP/IP_SVE_FP16.h b/src/VecSim/spaces/IP/IP_SVE_FP16.h index ac464977e..ca938bf0a 100644 --- a/src/VecSim/spaces/IP/IP_SVE_FP16.h +++ b/src/VecSim/spaces/IP/IP_SVE_FP16.h @@ -6,17 +6,44 @@ * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the * GNU Affero General Public License v3 (AGPLv3). */ +#pragma once #include -inline void InnerProduct_Step(const float16_t *vec1, const float16_t *vec2, svfloat16_t &acc, - size_t &offset, const size_t chunk) { +// Widen a half-precision vector into two single-precision vectors. `svcvt_f32_f16` reads each +// input from the bottom 16 bits of a 32-bit lane, and ZIP1/ZIP2 interleave the lower and upper +// halves of their operands, so zipping against zero leaves `lo` holding the vector's lower +// contiguous half (elements 0 .. N/2-1) and `hi` the upper half, one value per 32-bit lane. +// Splitting this way changes the summation order relative to a scalar loop, and fp32 addition still +// rounds, so the result can differ in the last bits. That is true of every accumulator split in +// these kernels, which is why the randomized tests compare against a tolerance. +inline void widen_f16_to_f32(svfloat16_t v, svfloat32_t &lo, svfloat32_t &hi) { + const svfloat16_t zero = svdup_f16(0.0f); + lo = svcvt_f32_f16_x(svptrue_b32(), svzip1_f16(v, zero)); + hi = svcvt_f32_f16_x(svptrue_b32(), svzip2_f16(v, zero)); +} + +// The products accumulate in fp32, not fp16. `FP16_InnerProduct` widens each stored half with +// FP16_to_FP32 and accumulates into a float, so a half precision accumulator would make this tier +// disagree with the same function computed anywhere else. It is also unsafe: 65504 is the largest +// finite fp16 value, so 32 elements of 200.0, all ordinary fp16 values, drive a half precision +// accumulator past it and the result becomes infinity. An fp32 accumulator cannot overflow for +// any fp16 input. +inline void InnerProduct_Step(const float16_t *vec1, const float16_t *vec2, svfloat32_t &acc_lo, + svfloat32_t &acc_hi, size_t &offset, const size_t chunk) { svbool_t all = svptrue_b16(); + svbool_t all32 = svptrue_b32(); // Load half-precision vectors. svfloat16_t v1 = svld1_f16(all, vec1 + offset); svfloat16_t v2 = svld1_f16(all, vec2 + offset); - // Compute multiplications and add to the accumulator - acc = svmla_f16_x(all, acc, v1, v2); + + svfloat32_t a_lo, a_hi, b_lo, b_hi; + widen_f16_to_f32(v1, a_lo, a_hi); + widen_f16_to_f32(v2, b_lo, b_hi); + + // Compute multiplications and add to the accumulators, in single precision. + acc_lo = svmla_f32_x(all32, acc_lo, a_lo, b_lo); + acc_hi = svmla_f32_x(all32, acc_hi, a_hi, b_hi); // Move to next chunk offset += chunk; @@ -27,48 +54,55 @@ float FP16_InnerProduct_SVE(const void *pVect1v, const void *pVect2v, size_t dim const auto *vec1 = static_cast(pVect1v); const auto *vec2 = static_cast(pVect2v); const size_t chunk = svcnth(); // number of 16-bit elements in a register - svbool_t all = svptrue_b16(); - svfloat16_t acc1 = svdup_f16(0.0f); - svfloat16_t acc2 = svdup_f16(0.0f); - svfloat16_t acc3 = svdup_f16(0.0f); - svfloat16_t acc4 = svdup_f16(0.0f); + // Four accumulator pairs, keeping the original four-way unrolling with fp32 accumulators. + svfloat32_t acc1_lo = svdup_f32(0.0f), acc1_hi = svdup_f32(0.0f); + svfloat32_t acc2_lo = svdup_f32(0.0f), acc2_hi = svdup_f32(0.0f); + svfloat32_t acc3_lo = svdup_f32(0.0f), acc3_hi = svdup_f32(0.0f); + svfloat32_t acc4_lo = svdup_f32(0.0f), acc4_hi = svdup_f32(0.0f); size_t offset = 0; // Process all full vectors const size_t full_iterations = dimension / chunk / 4; for (size_t iter = 0; iter < full_iterations; iter++) { - InnerProduct_Step(vec1, vec2, acc1, offset, chunk); - InnerProduct_Step(vec1, vec2, acc2, offset, chunk); - InnerProduct_Step(vec1, vec2, acc3, offset, chunk); - InnerProduct_Step(vec1, vec2, acc4, offset, chunk); + InnerProduct_Step(vec1, vec2, acc1_lo, acc1_hi, offset, chunk); + InnerProduct_Step(vec1, vec2, acc2_lo, acc2_hi, offset, chunk); + InnerProduct_Step(vec1, vec2, acc3_lo, acc3_hi, offset, chunk); + InnerProduct_Step(vec1, vec2, acc4_lo, acc4_hi, offset, chunk); } // Perform between 0 and 3 additional steps, according to `additional_steps` value if constexpr (additional_steps >= 1) - InnerProduct_Step(vec1, vec2, acc1, offset, chunk); + InnerProduct_Step(vec1, vec2, acc1_lo, acc1_hi, offset, chunk); if constexpr (additional_steps >= 2) - InnerProduct_Step(vec1, vec2, acc2, offset, chunk); + InnerProduct_Step(vec1, vec2, acc2_lo, acc2_hi, offset, chunk); if constexpr (additional_steps >= 3) - InnerProduct_Step(vec1, vec2, acc3, offset, chunk); + InnerProduct_Step(vec1, vec2, acc3_lo, acc3_hi, offset, chunk); // Handle the tail with the residual predicate if constexpr (partial_chunk) { svbool_t pg = svwhilelt_b16_u64(offset, dimension); - // Load half-precision vectors. + // The predicated load zeroes the inactive lanes, so the arithmetic below can run + // unpredicated: those lanes contribute a zero product. svfloat16_t v1 = svld1_f16(pg, vec1 + offset); svfloat16_t v2 = svld1_f16(pg, vec2 + offset); - // Compute multiplications and add to the accumulator. - // use the existing value of `acc` for the inactive elements (by the `m` suffix) - acc4 = svmla_f16_m(pg, acc4, v1, v2); + + svfloat32_t a_lo, a_hi, b_lo, b_hi; + widen_f16_to_f32(v1, a_lo, a_hi); + widen_f16_to_f32(v2, b_lo, b_hi); + + svbool_t all32 = svptrue_b32(); + acc4_lo = svmla_f32_x(all32, acc4_lo, a_lo, b_lo); + acc4_hi = svmla_f32_x(all32, acc4_hi, a_hi, b_hi); } - // Accumulate accumulators - acc1 = svadd_f16_x(all, acc1, acc3); - acc2 = svadd_f16_x(all, acc2, acc4); - acc1 = svadd_f16_x(all, acc1, acc2); + // Accumulate accumulators, all in fp32. + svbool_t all32 = svptrue_b32(); + svfloat32_t sum = svadd_f32_x(all32, svadd_f32_x(all32, acc1_lo, acc1_hi), + svadd_f32_x(all32, acc2_lo, acc2_hi)); + sum = svadd_f32_x(all32, sum, svadd_f32_x(all32, acc3_lo, acc3_hi)); + sum = svadd_f32_x(all32, sum, svadd_f32_x(all32, acc4_lo, acc4_hi)); // Reduce the accumulated sum. - float result = svaddv_f16(all, acc1); - return 1.0f - result; + return 1.0f - svaddv_f32(all32, sum); } diff --git a/src/VecSim/spaces/L2/L2_AVX512FP16_VL_FP16.h b/src/VecSim/spaces/L2/L2_AVX512FP16_VL_FP16.h index 27e909a30..d4d59d33d 100644 --- a/src/VecSim/spaces/L2/L2_AVX512FP16_VL_FP16.h +++ b/src/VecSim/spaces/L2/L2_AVX512FP16_VL_FP16.h @@ -6,6 +6,7 @@ * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the * GNU Affero General Public License v3 (AGPLv3). */ +#pragma once #include #include "VecSim/spaces/space_includes.h" #include "VecSim/types/float16.h" @@ -13,15 +14,23 @@ using float16 = vecsim_types::float16; -static inline void L2SqrStep(float16 *&pVect1, float16 *&pVect2, __m512h &sum) { - __m512h v1 = _mm512_loadu_ph(pVect1); - __m512h v2 = _mm512_loadu_ph(pVect2); +// The arithmetic here is fp32, not fp16. `FP16_L2Sqr` widens each stored half with FP16_to_FP32 and +// accumulates into a float, so accumulating in half precision would make this tier disagree with +// the same function computed anywhere else. It is also unsafe: 65504 is the largest finite fp16 +// value, so 32 elements of 200.0, all ordinary fp16 values, drive a half precision accumulator +// past it and the result becomes infinity. An fp32 accumulator cannot overflow for any fp16 input. +// This mirrors L2_AVX512F_FP16.h step for step; after widening there is nothing +// half-precision-specific left to do differently. +static inline void L2SqrStep(float16 *&pVect1, float16 *&pVect2, __m512 &sum) { + // Convert 16 half-floats into floats and store them in 512 bits register. + auto v1 = _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect1)); + auto v2 = _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect2)); - __m512h diff = _mm512_sub_ph(v1, v2); - - sum = _mm512_fmadd_ph(diff, diff, sum); - pVect1 += 32; - pVect2 += 32; + // sum = (v1 - v2)^2 + sum + auto c = _mm512_sub_ps(v1, v2); + sum = _mm512_fmadd_ps(c, c, sum); + pVect1 += 16; + pVect2 += 16; } template // 0..31 @@ -31,24 +40,33 @@ float FP16_L2SqrSIMD32_AVX512FP16_VL(const void *pVect1v, const void *pVect2v, s const float16 *pEnd1 = pVect1 + dimension; - __m512h sum = _mm512_setzero_ph(); - - if constexpr (residual) { - constexpr __mmask32 mask = (1LU << residual) - 1; - __m512h v1 = _mm512_loadu_ph(pVect1); - pVect1 += residual; - __m512h v2 = _mm512_loadu_ph(pVect2); - pVect2 += residual; - __m512h diff = _mm512_maskz_sub_ph(mask, v1, v2); + // Two accumulators break the FMA dependency chain, letting more FMAs be in flight at once. + auto sum0 = _mm512_setzero_ps(); + auto sum1 = _mm512_setzero_ps(); - sum = _mm512_mul_ph(diff, diff); + if constexpr (residual % 16) { + // Deal with remainder first. The full-width load of 16 16-bit floats is safe because + // `dim` is at least 16, so the vector spans at least 16 elements. + __mmask16 constexpr residuals_mask = (1 << (residual % 16)) - 1; + auto v1 = _mm512_maskz_mov_ps(residuals_mask, + _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect1))); + auto v2 = _mm512_maskz_mov_ps(residuals_mask, + _mm512_cvtph_ps(_mm256_lddqu_si256((__m256i *)pVect2))); + auto c = _mm512_sub_ps(v1, v2); + sum0 = _mm512_mul_ps(c, c); + pVect1 += residual % 16; + pVect2 += residual % 16; + } + // Handle the remaining full 16-element block of the residual (compile-time resolved). + if constexpr (residual >= 16) { + L2SqrStep(pVect1, pVect2, sum1); } // We dealt with the residual part. We are left with some multiple of 32 16-bit floats. - do { - L2SqrStep(pVect1, pVect2, sum); - } while (pVect1 < pEnd1); + while (pVect1 < pEnd1) { + L2SqrStep(pVect1, pVect2, sum0); + L2SqrStep(pVect1, pVect2, sum1); + } - _Float16 res = _mm512_reduce_add_ph(sum); - return res; + return _mm512_reduce_add_ps(_mm512_add_ps(sum0, sum1)); } diff --git a/src/VecSim/spaces/L2/L2_NEON_FP16.h b/src/VecSim/spaces/L2/L2_NEON_FP16.h index e2786aa7a..52b30e283 100644 --- a/src/VecSim/spaces/L2/L2_NEON_FP16.h +++ b/src/VecSim/spaces/L2/L2_NEON_FP16.h @@ -6,19 +6,28 @@ * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the * GNU Affero General Public License v3 (AGPLv3). */ +#pragma once #include -inline void L2Sqr_Step(const float16_t *&vec1, const float16_t *&vec2, float16x8_t &acc) { +// The squared differences accumulate in fp32, not fp16. `FP16_L2Sqr` widens each stored half with +// FP16_to_FP32, subtracts in float and accumulates into a float, so a half precision accumulator +// would make this tier disagree with the same function computed anywhere else. It is also unsafe: +// 65504 is the largest finite fp16 value, so 32 elements of 200.0, all ordinary fp16 values, drive +// a half precision accumulator past it and the result becomes infinity. An fp32 accumulator cannot +// overflow for any fp16 input. +inline void L2Sqr_Step(const float16_t *&vec1, const float16_t *&vec2, float32x4_t &acc_lo, + float32x4_t &acc_hi) { // Load half-precision vectors float16x8_t v1 = vld1q_f16(vec1); float16x8_t v2 = vld1q_f16(vec2); vec1 += 8; vec2 += 8; - // Calculate differences - float16x8_t diff = vsubq_f16(v1, v2); - // Square and accumulate - acc = vfmaq_f16(acc, diff, diff); + // Widen both halves to single precision, then subtract and accumulate in fp32. + float32x4_t d_lo = vsubq_f32(vcvt_f32_f16(vget_low_f16(v1)), vcvt_f32_f16(vget_low_f16(v2))); + float32x4_t d_hi = vsubq_f32(vcvt_high_f32_f16(v1), vcvt_high_f32_f16(v2)); + acc_lo = vfmaq_f32(acc_lo, d_lo, d_lo); + acc_hi = vfmaq_f32(acc_hi, d_hi, d_hi); } template // 0..31 @@ -26,10 +35,11 @@ float FP16_L2Sqr_NEON_HP(const void *pVect1v, const void *pVect2v, size_t dimens const auto *vec1 = static_cast(pVect1v); const auto *vec2 = static_cast(pVect2v); const auto *const v1End = vec1 + dimension; - float16x8_t acc1 = vdupq_n_f16(0.0f); - float16x8_t acc2 = vdupq_n_f16(0.0f); - float16x8_t acc3 = vdupq_n_f16(0.0f); - float16x8_t acc4 = vdupq_n_f16(0.0f); + // Four accumulator pairs, keeping the original four-way unrolling with fp32 accumulators. + float32x4_t acc1_lo = vdupq_n_f32(0.0f), acc1_hi = vdupq_n_f32(0.0f); + float32x4_t acc2_lo = vdupq_n_f32(0.0f), acc2_hi = vdupq_n_f32(0.0f); + float32x4_t acc3_lo = vdupq_n_f32(0.0f), acc3_hi = vdupq_n_f32(0.0f); + float32x4_t acc4_lo = vdupq_n_f32(0.0f), acc4_hi = vdupq_n_f32(0.0f); // First, handle the partial chunk residual if constexpr (residual % 8) { @@ -51,13 +61,16 @@ float FP16_L2Sqr_NEON_HP(const void *pVect1v, const void *pVect2v, size_t dimens float16x8_t v2 = vld1q_f16(vec2); // Apply mask to both vectors - float16x8_t masked_v1 = vbslq_f16(mask, v1, acc1); // `acc1` should be all zeros here - float16x8_t masked_v2 = vbslq_f16(mask, v2, acc2); // `acc2` should be all zeros here + const float16x8_t zero_h = vdupq_n_f16(0.0f); + float16x8_t masked_v1 = vbslq_f16(mask, v1, zero_h); + float16x8_t masked_v2 = vbslq_f16(mask, v2, zero_h); - // Calculate differences - float16x8_t diff = vsubq_f16(masked_v1, masked_v2); - // Square and accumulate - acc1 = vfmaq_f16(acc1, diff, diff); + // Widen, subtract and accumulate in fp32; the masked lanes contribute zero. + float32x4_t d_lo = + vsubq_f32(vcvt_f32_f16(vget_low_f16(masked_v1)), vcvt_f32_f16(vget_low_f16(masked_v2))); + float32x4_t d_hi = vsubq_f32(vcvt_high_f32_f16(masked_v1), vcvt_high_f32_f16(masked_v2)); + acc1_lo = vfmaq_f32(acc1_lo, d_lo, d_lo); + acc1_hi = vfmaq_f32(acc1_hi, d_hi, d_hi); // Advance pointers vec1 += chunk_residual; @@ -66,34 +79,26 @@ float FP16_L2Sqr_NEON_HP(const void *pVect1v, const void *pVect2v, size_t dimens // Handle (residual - (residual % 8)) in chunks of 8 float16 if constexpr (residual >= 8) - L2Sqr_Step(vec1, vec2, acc2); + L2Sqr_Step(vec1, vec2, acc2_lo, acc2_hi); if constexpr (residual >= 16) - L2Sqr_Step(vec1, vec2, acc3); + L2Sqr_Step(vec1, vec2, acc3_lo, acc3_hi); if constexpr (residual >= 24) - L2Sqr_Step(vec1, vec2, acc4); + L2Sqr_Step(vec1, vec2, acc4_lo, acc4_hi); // Process the rest of the vectors (the full chunks part) while (vec1 < v1End) { // TODO: use `vld1q_f16_x4` for quad-loading? - L2Sqr_Step(vec1, vec2, acc1); - L2Sqr_Step(vec1, vec2, acc2); - L2Sqr_Step(vec1, vec2, acc3); - L2Sqr_Step(vec1, vec2, acc4); + L2Sqr_Step(vec1, vec2, acc1_lo, acc1_hi); + L2Sqr_Step(vec1, vec2, acc2_lo, acc2_hi); + L2Sqr_Step(vec1, vec2, acc3_lo, acc3_hi); + L2Sqr_Step(vec1, vec2, acc4_lo, acc4_hi); } - // Accumulate accumulators - acc1 = vpaddq_f16(acc1, acc3); - acc2 = vpaddq_f16(acc2, acc4); - acc1 = vpaddq_f16(acc1, acc2); - - // Horizontal sum of the accumulated values - float32x4_t sum_f32 = vcvt_f32_f16(vget_low_f16(acc1)); - sum_f32 = vaddq_f32(sum_f32, vcvt_f32_f16(vget_high_f16(acc1))); - - // Pairwise add to get horizontal sum - float32x2_t sum_2 = vadd_f32(vget_low_f32(sum_f32), vget_high_f32(sum_f32)); - sum_2 = vpadd_f32(sum_2, sum_2); + // Accumulate accumulators, all in fp32. + float32x4_t sum_f32 = vaddq_f32(vaddq_f32(acc1_lo, acc1_hi), vaddq_f32(acc2_lo, acc2_hi)); + sum_f32 = vaddq_f32(sum_f32, vaddq_f32(acc3_lo, acc3_hi)); + sum_f32 = vaddq_f32(sum_f32, vaddq_f32(acc4_lo, acc4_hi)); // Extract result - return vget_lane_f32(sum_2, 0); + return vaddvq_f32(sum_f32); } diff --git a/src/VecSim/spaces/L2/L2_SVE_FP16.h b/src/VecSim/spaces/L2/L2_SVE_FP16.h index 24b5ee2df..f0d6a7d8d 100644 --- a/src/VecSim/spaces/L2/L2_SVE_FP16.h +++ b/src/VecSim/spaces/L2/L2_SVE_FP16.h @@ -6,18 +6,34 @@ * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the * GNU Affero General Public License v3 (AGPLv3). */ +#pragma once #include +// widen_f16_to_f32 lives here: SVE.cpp compiles both headers, so it must be defined once. +#include "VecSim/spaces/IP/IP_SVE_FP16.h" -inline void L2Sqr_Step(const float16_t *vec1, const float16_t *vec2, svfloat16_t &acc, - size_t &offset, const size_t chunk) { +// The squared differences accumulate in fp32, not fp16. `FP16_L2Sqr` widens each stored half with +// FP16_to_FP32, subtracts in float and accumulates into a float, so a half precision accumulator +// would make this tier disagree with the same function computed anywhere else. It is also unsafe: +// 65504 is the largest finite fp16 value, so 32 elements of 200.0, all ordinary fp16 values, drive +// a half precision accumulator past it and the result becomes infinity. An fp32 accumulator cannot +// overflow for any fp16 input. +inline void L2Sqr_Step(const float16_t *vec1, const float16_t *vec2, svfloat32_t &acc_lo, + svfloat32_t &acc_hi, size_t &offset, const size_t chunk) { svbool_t all = svptrue_b16(); + svbool_t all32 = svptrue_b32(); svfloat16_t v1 = svld1_f16(all, vec1 + offset); svfloat16_t v2 = svld1_f16(all, vec2 + offset); - // Compute difference in half precision. - svfloat16_t diff = svsub_f16_x(all, v1, v2); - // Square the differences and accumulate - acc = svmla_f16_x(all, acc, diff, diff); + + svfloat32_t a_lo, a_hi, b_lo, b_hi; + widen_f16_to_f32(v1, a_lo, a_hi); + widen_f16_to_f32(v2, b_lo, b_hi); + + // Subtract and accumulate in single precision. + svfloat32_t d_lo = svsub_f32_x(all32, a_lo, b_lo); + svfloat32_t d_hi = svsub_f32_x(all32, a_hi, b_hi); + acc_lo = svmla_f32_x(all32, acc_lo, d_lo, d_lo); + acc_hi = svmla_f32_x(all32, acc_hi, d_hi, d_hi); offset += chunk; } @@ -26,50 +42,57 @@ float FP16_L2Sqr_SVE(const void *pVect1v, const void *pVect2v, size_t dimension) const auto *vec1 = static_cast(pVect1v); const auto *vec2 = static_cast(pVect2v); const size_t chunk = svcnth(); // number of 16-bit elements in a register - svbool_t all = svptrue_b16(); - svfloat16_t acc1 = svdup_f16(0.0f); - svfloat16_t acc2 = svdup_f16(0.0f); - svfloat16_t acc3 = svdup_f16(0.0f); - svfloat16_t acc4 = svdup_f16(0.0f); + // Four accumulator pairs, keeping the original four-way unrolling with fp32 accumulators. + svfloat32_t acc1_lo = svdup_f32(0.0f), acc1_hi = svdup_f32(0.0f); + svfloat32_t acc2_lo = svdup_f32(0.0f), acc2_hi = svdup_f32(0.0f); + svfloat32_t acc3_lo = svdup_f32(0.0f), acc3_hi = svdup_f32(0.0f); + svfloat32_t acc4_lo = svdup_f32(0.0f), acc4_hi = svdup_f32(0.0f); size_t offset = 0; // Process all full vectors const size_t full_iterations = dimension / chunk / 4; for (size_t iter = 0; iter < full_iterations; iter++) { - L2Sqr_Step(vec1, vec2, acc1, offset, chunk); - L2Sqr_Step(vec1, vec2, acc2, offset, chunk); - L2Sqr_Step(vec1, vec2, acc3, offset, chunk); - L2Sqr_Step(vec1, vec2, acc4, offset, chunk); + L2Sqr_Step(vec1, vec2, acc1_lo, acc1_hi, offset, chunk); + L2Sqr_Step(vec1, vec2, acc2_lo, acc2_hi, offset, chunk); + L2Sqr_Step(vec1, vec2, acc3_lo, acc3_hi, offset, chunk); + L2Sqr_Step(vec1, vec2, acc4_lo, acc4_hi, offset, chunk); } // Perform between 0 and 3 additional steps, according to `additional_steps` value if constexpr (additional_steps >= 1) - L2Sqr_Step(vec1, vec2, acc1, offset, chunk); + L2Sqr_Step(vec1, vec2, acc1_lo, acc1_hi, offset, chunk); if constexpr (additional_steps >= 2) - L2Sqr_Step(vec1, vec2, acc2, offset, chunk); + L2Sqr_Step(vec1, vec2, acc2_lo, acc2_hi, offset, chunk); if constexpr (additional_steps >= 3) - L2Sqr_Step(vec1, vec2, acc3, offset, chunk); + L2Sqr_Step(vec1, vec2, acc3_lo, acc3_hi, offset, chunk); // Handle partial chunk, if needed if constexpr (partial_chunk) { svbool_t pg = svwhilelt_b16_u64(offset, dimension); - // Load half-precision vectors. + // Load half-precision vectors. The predicated load zeroes the inactive lanes, so the + // arithmetic below can run unpredicated: those lanes contribute a squared zero. svfloat16_t v1 = svld1_f16(pg, vec1 + offset); svfloat16_t v2 = svld1_f16(pg, vec2 + offset); - // Compute difference in half precision. - svfloat16_t diff = svsub_f16_x(pg, v1, v2); - // Square the differences. - // Use `m` suffix to keep the inactive elements as they are in `acc` - acc4 = svmla_f16_m(pg, acc4, diff, diff); + + svfloat32_t a_lo, a_hi, b_lo, b_hi; + widen_f16_to_f32(v1, a_lo, a_hi); + widen_f16_to_f32(v2, b_lo, b_hi); + + svbool_t all32 = svptrue_b32(); + svfloat32_t d_lo = svsub_f32_x(all32, a_lo, b_lo); + svfloat32_t d_hi = svsub_f32_x(all32, a_hi, b_hi); + acc4_lo = svmla_f32_x(all32, acc4_lo, d_lo, d_lo); + acc4_hi = svmla_f32_x(all32, acc4_hi, d_hi, d_hi); } - // Accumulate accumulators - acc1 = svadd_f16_x(all, acc1, acc3); - acc2 = svadd_f16_x(all, acc2, acc4); - acc1 = svadd_f16_x(all, acc1, acc2); + // Accumulate accumulators, all in fp32. + svbool_t all32 = svptrue_b32(); + svfloat32_t sum = svadd_f32_x(all32, svadd_f32_x(all32, acc1_lo, acc1_hi), + svadd_f32_x(all32, acc2_lo, acc2_hi)); + sum = svadd_f32_x(all32, sum, svadd_f32_x(all32, acc3_lo, acc3_hi)); + sum = svadd_f32_x(all32, sum, svadd_f32_x(all32, acc4_lo, acc4_hi)); // Reduce the accumulated sum. - float result = svaddv_f16(all, acc1); - return result; + return svaddv_f32(all32, sum); } diff --git a/tests/unit/test_spaces.cpp b/tests/unit/test_spaces.cpp index 1175fd324..f85df976e 100644 --- a/tests/unit/test_spaces.cpp +++ b/tests/unit/test_spaces.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include "gtest/gtest.h" #include "VecSim/spaces/space_includes.h" @@ -1427,12 +1428,11 @@ TEST_P(FP16SpacesOptimizationTestAdvanced, FP16InnerProductTestAdv) { std::mt19937 gen(42); std::uniform_real_distribution<> dis(-0.99, 0.99); -#if defined(CPU_FEATURES_ARCH_AARCH64) && defined(__GNUC__) && (__GNUC__ < 13) - // https://github.com/pytorch/executorch/issues/6844 - __fp16 baseline = 0; -#else - _Float16 baseline = 0; -#endif + // Accumulate the reference in float, exactly as FP16_InnerProduct does: it widens each stored + // half with FP16_to_FP32 and sums into a float. A _Float16 accumulator here would make this + // test agree with any kernel that also accumulates in half precision, which is how an fp16 + // accumulator in these tiers went unnoticed. + float baseline = 0; for (size_t i = 0; i < dim; i++) { float val1 = (dis(gen)); @@ -1440,9 +1440,9 @@ TEST_P(FP16SpacesOptimizationTestAdvanced, FP16InnerProductTestAdv) { v1[i] = vecsim_types::FP32_to_FP16((val1)); v2[i] = vecsim_types::FP32_to_FP16((val2)); - baseline += static_cast(val1) * static_cast(val2); + baseline += vecsim_types::FP16_to_FP32(v1[i]) * vecsim_types::FP16_to_FP32(v2[i]); } - baseline = decltype(baseline)(1) - baseline; + baseline = 1.0f - baseline; auto expected_alignment = [](size_t reg_bit_size, size_t dim) { size_t elements_in_reg = reg_bit_size / sizeof(float16) / 8; @@ -1532,14 +1532,17 @@ TEST_P(FP16SpacesOptimizationTestAdvanced, FP16L2SqrTestAdv) { std::mt19937 gen(42); std::uniform_real_distribution dis(-0.99f, 0.99f); - _Float16 baseline = 0; + // Accumulate the reference in float, exactly as FP16_L2Sqr does: widen both stored halves + // with FP16_to_FP32, subtract in float, and sum into a float. See the note on the inner + // product baseline above. + float baseline = 0; for (size_t i = 0; i < dim; i++) { float val1 = (dis(gen)); float val2 = (dis(gen)); v1[i] = vecsim_types::FP32_to_FP16((val1)); v2[i] = vecsim_types::FP32_to_FP16((val2)); - _Float16 diff = static_cast<_Float16>(val1) - static_cast<_Float16>(val2); + float diff = vecsim_types::FP16_to_FP32(v1[i]) - vecsim_types::FP16_to_FP32(v2[i]); baseline += diff * diff; } @@ -1570,6 +1573,47 @@ INSTANTIATE_TEST_SUITE_P(, FP16SpacesOptimizationTestAdvanced, #endif // defined(OPT_AVX512_FP16_VL) || defined(CPU_FEATURES_ARCH_AARCH64) +// Regression test for the accumulator width of the float16 SIMD kernels. +// +// The randomized tests above cannot catch a half precision accumulator. They draw values from +// [-0.99, 0.99] and allow 1% relative error, and a kernel that accumulates in fp16 stays inside +// that budget: its worst error over dim 32..256 on that distribution is about 0.54%. These inputs +// separate the two implementations by a wide margin instead. 65504 is the largest finite fp16 +// value, so summing even a handful of squared differences of 200 pushes a half precision +// accumulator past it and the result becomes infinity, while every value here is exactly +// representable in fp32 and the expected total is exact. +// +// The public choosers are called without a feature override, so whichever tier this CPU selects is +// the one under test, and the scalar path is covered on machines with no SIMD tier at all. +TEST(FP16SpacesTest, LargeValuesDoNotOverflowTheAccumulator) { + // Exactly `dim` elements, no padding: the residual paths issue full-width loads that overlap + // the prefix rather than reading past the end, so at dim=35 the x86 kernel covers 0..15, then + // 3..18 and 19..34. Tight buffers keep ASan's bounds checking meaningful here. + for (size_t dim : {32UL, 35UL, 40UL, 64UL, 128UL}) { + std::vector v1(dim, vecsim_types::FP32_to_FP16(0.0f)); + std::vector v2(dim, vecsim_types::FP32_to_FP16(0.0f)); + for (size_t i = 0; i < dim; i++) { + v1[i] = vecsim_types::FP32_to_FP16(200.0f); + } + + // L2: v2 is all zeros, so the distance is dim * 200^2. + const float expected_l2 = static_cast(dim) * 200.0f * 200.0f; + const float l2 = L2_FP16_GetDistFunc(dim)(v1.data(), v2.data(), dim); + ASSERT_TRUE(std::isfinite(l2)) << "L2 accumulator overflowed at dim " << dim; + ASSERT_NEAR(l2, expected_l2, expected_l2 * 1e-6f) << "L2 at dim " << dim; + + // Inner product: both sides at 200, so the raw product sums to dim * 200^2 and the + // returned distance is 1 - that. + for (size_t i = 0; i < dim; i++) { + v2[i] = vecsim_types::FP32_to_FP16(200.0f); + } + const float expected_ip = 1.0f - static_cast(dim) * 200.0f * 200.0f; + const float ip = IP_FP16_GetDistFunc(dim)(v1.data(), v2.data(), dim); + ASSERT_TRUE(std::isfinite(ip)) << "IP accumulator overflowed at dim " << dim; + ASSERT_NEAR(ip, expected_ip, std::abs(expected_ip) * 1e-6f) << "IP at dim " << dim; + } +} + class INT8SpacesOptimizationTest : public testing::TestWithParam {}; TEST_P(INT8SpacesOptimizationTest, INT8L2SqrTest) { From 57cba151aad5b0a2b544d3297fb270f7ade3ef48 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 23 Aug 2026 14:37:52 +0300 Subject: [PATCH 10/14] Add the ISA feature and tier manifests Two declarative tables that will become the single source of truth for the SIMD dispatch layer. Nothing consumes them yet: this commit only states, in one place and in one vocabulary, what is currently spread across cmake/*InstructionFlags.cmake, spaces/CMakeLists.txt, and about 40 hand-written chooser bodies. isa_features.def has one row per CPU feature: the token tiers name, its architecture, the compiler flag fragment it contributes, the lowest -march level that admits that fragment, and the cpu_features field naming the runtime bit. Three relations are spelled distinctly rather than as one "implies", because they answer different questions: ARCH_IMPLIES enumerates which CPU masks are possible, FLAG_ENABLES records what a fragment forces the compiler to assume, and LEVEL_REQUIRES what raising -march forces. isa_tiers.def has one row per tier, with FLAGS_FROM and GUARANTEES as separate columns. That separation is the point of the whole design: a flag fragment expands to a compiler-defined bundle, so compiling with a flag does not tell you which bits the deployment CPU must have. Two live bugs came from conflating them, and both are now visible as a column difference instead of being invisible. Every row was derived by reading the build rather than by inference, and three things that reading corrected: - The tier set is 23, matching the 23 set_source_files_properties() entries. An earlier count of 20 came from grepping OPT_* in the two dispatch files, which misses tiers guarded by another tier's macro. - AVX2_F16C, AVX2_FMA_F16C and SSE4_F16C are tiers in their own right, not variants of AVX2, AVX2_FMA and SSE4. Each is its own translation unit with its own flags and predicate, carrying the SQ8-to-FP16 kernels that need vcvtph2ps, while the plain tiers carry the SQ8-to-FP32 kernels that do not. Legacy code expressed this as one shared OPT_F16C guard around three differently predicated branches, which is why they look like variants. - The F16C and SSE4_F16C predicates include bits beyond the tier's nominal feature: f16c && fma3 && avx and sse4_1 && f16c && avx respectively. -mf16c is VEX-encoded, so the flag puts AVX-state instructions in reach and the predicates already account for it. Priorities are spaced by 10 so a tier can be inserted without renumbering, and they reproduce the legacy selection order for both SQ8_FP32 (VNNI, AVX2_FMA, AVX2, SSE4) and SQ8_FP16 (AVX512F, AVX2_FMA_F16C, AVX2_F16C, SSE4_F16C). A parity test will pin that rather than leaving it asserted here. --- src/VecSim/spaces/isa_features.def | 90 ++++++++++++++++++++++++++++++ src/VecSim/spaces/isa_tiers.def | 72 ++++++++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 src/VecSim/spaces/isa_features.def create mode 100644 src/VecSim/spaces/isa_tiers.def diff --git a/src/VecSim/spaces/isa_features.def b/src/VecSim/spaces/isa_features.def new file mode 100644 index 000000000..e1249731a --- /dev/null +++ b/src/VecSim/spaces/isa_features.def @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2006-Present, Redis Ltd. + * All rights reserved. + * + * Licensed under your choice of the Redis Source Available License 2.0 + * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the + * GNU Affero General Public License v3 (AGPLv3). + */ + +// Feature-token table: one row per CPU feature, one physical line per row. A feature token is the +// only thing a tier in isa_tiers.def is allowed to name. +// +// Each row carries everything needed to derive a tier's compiler flags from its token list alone, +// and to state that tier's runtime predicate in the same vocabulary. Those stay separate axes on +// purpose: this library is built on one machine and run on another, so what the compiler was +// allowed to emit and what the running CPU implements are different questions. A flag fragment +// expands to a compiler-defined bundle that differs between gcc and clang for the same -march +// string, so a runtime predicate is never derived from flags. +// +// Columns: +// token the name tiers use, unique across both architectures +// arch X86 or ARM; a tier may only name tokens of its own architecture +// flag fragment what this token contributes to the compile command. x86 unions the -m options; +// ARM appends "+feature" suffixes to one -march level, since ARM -march strings +// are monolithic +// min level lowest -march level admitting this fragment, or "-" on x86 where there is none. +// A tier's -march is the highest level in its token closure +// feature field the cpu_features struct field naming the runtime bit. This column, and only +// this column, feeds the runtime predicate +// +// There is deliberately no per-feature CXX_* probe column. The authoritative build gate is a +// whole-tier try_compile of the tier's real source under its complete derived flags, which subsumes +// per-flag probing and also catches a flag that passes alone but fails in the real combination. + +// token arch flag fragment min level feature field +FEATURE( SSE, X86, "-msse", -, sse ) +FEATURE( SSE3, X86, "-msse3", -, sse3 ) +FEATURE( SSE4_1, X86, "-msse4.1", -, sse4_1 ) +FEATURE( AVX, X86, "-mavx", -, avx ) +FEATURE( AVX2, X86, "-mavx2", -, avx2 ) +FEATURE( FMA3, X86, "-mfma", -, fma3 ) +FEATURE( F16C, X86, "-mf16c", -, f16c ) +FEATURE( AVX512F, X86, "-mavx512f", -, avx512f ) +FEATURE( AVX512BW, X86, "-mavx512bw", -, avx512bw ) +FEATURE( AVX512VL, X86, "-mavx512vl", -, avx512vl ) +FEATURE( AVX512VNNI, X86, "-mavx512vnni", -, avx512vnni ) +FEATURE( AVX512VBMI2, X86, "-mavx512vbmi2", -, avx512vbmi2 ) +FEATURE( AVX512BF16, X86, "-mavx512bf16", -, avx512_bf16 ) +FEATURE( AVX512FP16, X86, "-mavx512fp16", -, avx512_fp16 ) + +FEATURE( ASIMD, ARM, "", armv8-a, asimd ) +FEATURE( FPHP, ARM, "+fp16", armv8.2-a, fphp ) +FEATURE( ASIMDHP, ARM, "+fp16", armv8.2-a, asimdhp ) +FEATURE( ASIMDFHM, ARM, "+fp16fml", armv8.2-a, asimdfhm ) +FEATURE( ASIMDDP, ARM, "+dotprod", armv8.2-a, asimddp ) +FEATURE( BF16, ARM, "+bf16", armv8.2-a, bf16 ) +FEATURE( SVE, ARM, "+sve", armv8-a, sve ) +FEATURE( SVE2, ARM, "+sve2", armv9-a, sve2 ) +FEATURE( SVEBF16, ARM, "+bf16", armv8.2-a, svebf16 ) + +// Three distinct relations, spelled distinctly. A bare "implies" would conflate them, and they +// answer different questions: which CPU masks are possible, what a flag fragment forces the +// compiler to assume, and what raising the -march level forces. + +// ARCH_IMPLIES: if the left bit is set on a CPU then the right one is too. Used ONLY to enumerate +// the CPU masks worth testing, never to shorten a runtime predicate. +ARCH_IMPLIES( AVX2, AVX ) +ARCH_IMPLIES( AVX512BW, AVX512F ) +ARCH_IMPLIES( AVX512VL, AVX512F ) +ARCH_IMPLIES( AVX512VNNI, AVX512F ) +ARCH_IMPLIES( AVX512VBMI2, AVX512BW ) +ARCH_IMPLIES( AVX512BF16, AVX512F ) +ARCH_IMPLIES( AVX512FP16, AVX512F ) +ARCH_IMPLIES( SSE4_1, SSE3 ) +ARCH_IMPLIES( SSE3, SSE ) +ARCH_IMPLIES( ASIMDFHM, ASIMDHP ) +ARCH_IMPLIES( ASIMDHP, FPHP ) +ARCH_IMPLIES( SVE2, SVE ) +ARCH_IMPLIES( SVEBF16, SVE ) + +// FLAG_ENABLES: passing the left fragment makes the compiler assume the right capabilities, +// whatever the tier asked for. This is why a predicate cannot be derived from flags. F16C is +// VEX-encoded, so -mf16c forces AVX state, and "+fp16" is one fragment covering two HWCAPs. +FLAG_ENABLES( F16C, AVX ) +FLAG_ENABLES( "+fp16", ASIMDHP+FPHP ) +FLAG_ENABLES( "+bf16", BF16+SVEBF16 ) + +// LEVEL_REQUIRES: what raising the -march level itself forces, independent of any +fragment. +LEVEL_REQUIRES( armv8.1-a, ATOMICS+CRC32+QRDMX ) +LEVEL_REQUIRES( armv8.2-a, ATOMICS+CRC32+QRDMX ) diff --git a/src/VecSim/spaces/isa_tiers.def b/src/VecSim/spaces/isa_tiers.def new file mode 100644 index 000000000..91d002ea6 --- /dev/null +++ b/src/VecSim/spaces/isa_tiers.def @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2006-Present, Redis Ltd. + * All rights reserved. + * + * Licensed under your choice of the Redis Source Available License 2.0 + * (RSALv2); or (b) the Server Side Public License v1 (SSPLv1); or (c) the + * GNU Affero General Public License v3 (AGPLv3). + */ + +// Tier table: one row per SIMD tier, one physical line per row. Every token named here must exist +// in isa_features.def and belong to the same architecture as the tier. +// +// Columns: +// tier the tier's name in C++, as TierInfo and VECSIM_BUILT_ +// arch X86 or ARM +// stem file stem under spaces/functions/, so .cpp and .h. Deliberately the +// file stem and not the tier name, because the two differ today (tier +// AVX512_F_BW_VL_VNNI lives in AVX512F_BW_VL_VNNI.cpp) and renaming files is not +// this refactor's job +// priority higher wins when several tiers are eligible. Values are spaced by 10 so a tier can +// be inserted between two others without renumbering the table +// FLAGS_FROM tokens whose flag fragments build the compile command +// GUARANTEES tokens whose feature fields build the runtime predicate +// +// FLAGS_FROM and GUARANTEES are separate columns rather than one list, and this is the crux of the +// whole design. A flag fragment expands to a compiler-defined bundle, so compiling with a flag does +// not tell you which bits the deployment CPU must have. Two live bugs came from conflating them: a +// tier compiled at +fp16fml but dispatched on asimdhp alone emitted FMLAL into a path reachable on +// cores without FEAT_FHM, and six AVX512 VNNI dispatch sites omitted avx512vl while the tier was +// compiled with -mavx512vl. Where the columns differ, the difference is load-bearing and commented. + +// tier arch stem prio FLAGS_FROM GUARANTEES +TIER( AVX512BF16_VL, X86, AVX512BF16_VL, 100, (AVX512BF16, AVX512VL), (AVX512BF16, AVX512VL) ) +TIER( AVX512FP16_VL, X86, AVX512FP16_VL, 95, (AVX512FP16, AVX512VL), (AVX512FP16, AVX512VL) ) +TIER( AVX512_BW_VBMI2, X86, AVX512BW_VBMI2, 90, (AVX512BW, AVX512VBMI2), (AVX512BW, AVX512VBMI2) ) +// GUARANTEES lists AVX512VL because the tier is compiled with -mavx512vl and may therefore contain +// VL-encoded instructions. Six of the twelve legacy dispatch sites omitted that bit. +TIER( AVX512_F_BW_VL_VNNI, X86, AVX512F_BW_VL_VNNI, 85, (AVX512F, AVX512BW, AVX512VL, AVX512VNNI), (AVX512F, AVX512BW, AVX512VL, AVX512VNNI) ) +TIER( AVX512F, X86, AVX512F, 80, (AVX512F), (AVX512F) ) +// AVX2_F16C, AVX2_FMA_F16C and SSE4_F16C are separate tiers rather than variants of AVX2, AVX2_FMA +// and SSE4, because each is its own translation unit with its own flags and its own predicate. They +// carry the SQ8-to-FP16 kernels, which need vcvtph2ps and so need F16C, while the plain AVX2 and +// SSE4 tiers carry the SQ8-to-FP32 kernels that do not. Legacy code expressed this as one shared +// OPT_F16C guard wrapped around three differently-predicated branches. +TIER( AVX2_FMA_F16C, X86, AVX2_FMA_F16C, 70, (AVX2, FMA3, F16C), (AVX2, FMA3, F16C) ) +TIER( AVX2_FMA, X86, AVX2_FMA, 65, (AVX2, FMA3), (AVX2, FMA3) ) +TIER( AVX2_F16C, X86, AVX2_F16C, 60, (AVX2, F16C), (AVX2, F16C) ) +TIER( AVX2, X86, AVX2, 55, (AVX2), (AVX2) ) +// Compiled "-mf16c -mfma -mavx" and dispatched on f16c && fma3 && avx, so here the two columns +// agree. Worth stating because -mf16c is VEX-encoded and forces AVX state, so a tier naming only +// F16C would still be handed AVX by the compiler; this tier asks for all three explicitly. +TIER( F16C, X86, F16C, 50, (F16C, FMA3, AVX), (F16C, FMA3, AVX) ) +TIER( AVX, X86, AVX, 45, (AVX), (AVX) ) +// Compiled "-msse4.1 -mavx -mf16c" and dispatched on sse4_1 && f16c && avx. The AVX bit is in the +// predicate because the flag put AVX-state instructions in reach, not because the kernels asked +// for SSE4 plus AVX: this is FLAG_ENABLES(F16C, AVX) showing up in a real predicate. +TIER( SSE4_F16C, X86, SSE4_F16C, 40, (SSE4_1, AVX, F16C), (SSE4_1, F16C, AVX) ) +TIER( SSE4, X86, SSE4, 35, (SSE4_1), (SSE4_1) ) +TIER( SSE3, X86, SSE3, 30, (SSE3), (SSE3) ) +TIER( SSE, X86, SSE, 25, (SSE), (SSE) ) + +TIER( SVE2, ARM, SVE2, 100, (SVE2), (SVE2) ) +TIER( SVE_BF16, ARM, SVE_BF16, 95, (SVE, BF16), (SVE, SVEBF16) ) +TIER( SVE, ARM, SVE, 90, (SVE), (SVE) ) +TIER( NEON_BF16, ARM, NEON_BF16, 80, (BF16), (BF16) ) +// The tier that motivated splitting these columns. It is compiled at +fp16fml, so gcc contracts a +// widening convert plus FMA into FMLAL anywhere in the translation unit, and FEAT_FHM is optional in +// armv8.2-a. Dispatching it on asimdhp alone was a SIGILL on any core without FEAT_FHM. +TIER( NEON_FHM, ARM, NEON_FHM, 75, (ASIMDFHM), (ASIMDHP, ASIMDFHM) ) +TIER( NEON_HP, ARM, NEON_HP, 70, (ASIMDHP), (ASIMDHP) ) +TIER( NEON_DOTPROD, ARM, NEON_DOTPROD, 65, (ASIMDDP), (ASIMDDP) ) +TIER( NEON, ARM, NEON, 60, (ASIMD), (ASIMD) ) From 37f9a7bf3ec2ed04d2d547130f03753088802768 Mon Sep 17 00:00:00 2001 From: Dor Forer Date: Sun, 23 Aug 2026 14:45:53 +0300 Subject: [PATCH 11/14] Parse the ISA manifests in CMake and derive each tier's compile flags vecsim_manifest.cmake reads both .def files into CMake variables and derives a tier's compile flag string from its FLAGS_FROM column alone. x86 unions the -m fragments in token order; ARM emits one -march at the highest level among the tier's tokens, then each distinct +fragment, because ARM -march strings are monolithic rather than a union of independent options. GUARANTEES is parsed and carried through untouched, never consulted here. The runtime predicate is not derived from flags, because a fragment expands to a compiler-defined bundle that differs between compilers for the same -march string. Keeping the derivation one-directional is what stops this module from quietly recreating the unsound design. Verified: the derived flags are byte-for-byte identical to the 23 hardcoded strings in spaces/CMakeLists.txt today, compared by extracting both and diffing. That equality is the whole claim behind calling the manifest a source of truth, so it is worth stating as a measurement rather than an intention. Nothing consumes the derivation yet, so this commit cannot change what the build emits. Validation is deliberately fatal rather than skip-on-error: a tier naming an undefined token, a tier naming a token of the other architecture, an unknown ARM level, or a manifest that parses to zero rows all stop the configure. Each of those would otherwise disable tiers silently while the configure still succeeded, which is the failure mode this whole series has been about. tier_probe.cmake is carried over unchanged from the held P0.3 work: it try_compiles a tier's real source under the tier's real flags with CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY, since these translation units have no main() and a default executable probe would fail to link for every tier and disable the entire dispatch layer while reporting success. Two parser bugs found by running it rather than reading it. Commas are not CMake list separators, so the token columns arrived as single strings and foreach(IN LISTS) saw one element; they are now converted to semicolons. And the first version set CMP0057 to use IN_LIST, which leaks policy into the includer under CMP0011, so the membership tests use list(FIND) and the module needs no policy at all. --- cmake/tier_probe.cmake | 77 ++++++++++++++++++ cmake/vecsim_manifest.cmake | 154 ++++++++++++++++++++++++++++++++++++ 2 files changed, 231 insertions(+) create mode 100644 cmake/tier_probe.cmake create mode 100644 cmake/vecsim_manifest.cmake diff --git a/cmake/tier_probe.cmake b/cmake/tier_probe.cmake new file mode 100644 index 000000000..40df4d625 --- /dev/null +++ b/cmake/tier_probe.cmake @@ -0,0 +1,77 @@ +# Probe whether the current toolchain can actually build a SIMD tier, instead of inferring the +# toolchain's capability from a side channel (a compiler flag check alone, or a binutils version +# table). CHECK_CXX_COMPILER_FLAG only asks the compiler whether it recognizes a flag; it does +# not ask whether the compiler and assembler can carry a real translation unit through to a +# finished object under the tier's complete flag combination. This probe does that: it +# try_compiles the tier's own source file under the tier's own flags, so a flag combination that +# the compiler accepts individually but rejects together, or a flag whose instructions the +# assembler cannot emit, fails here rather than reaching the build. +# +# vecsim_tier_compiles( SOURCE FLAGS [NAME