[MOD-17845] Gate AVX512 VNNI dispatch on AVX512VL - #1019
Merged
Conversation
dor-forer
force-pushed
the
MOD-17845-simd-dispatch-hygiene
branch
from
August 20, 2026 08:35
30b4d10 to
49f3e8d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 49f3e8d. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## MOD-17844-arm-simd-isa-safety #1019 +/- ##
==============================================================
Coverage 97.18% 97.18%
==============================================================
Files 141 141
Lines 8537 8540 +3
==============================================================
+ Hits 8297 8300 +3
Misses 240 240 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NEON_HP.cpp was one translation unit compiled with -march=armv8.2-a+fp16fml, but its HP-only entry points were dispatched on features.asimdhp alone. Because the whole TU carried +fp16fml, the compiler was licensed to emit FMLAL/FMLSL instructions anywhere in it, including into the HP-only functions whose source has no FMLAL intrinsic. Measured on arm-r8g.xlarge with gcc 12: the HP-only wrappers compiled from identical source went from 32 AdvSIMD FMLAL/FMLSL instructions at +fp16fml down to 0 once compiled at +fp16 alone. On a core with asimdhp but without asimdfhm, the old HP path was therefore a SIGILL. Tightening the predicate to require both asimdhp and asimdfhm would not have fixed this: it would have deleted the HP-only fallback for exactly the CPUs that need it. The fix is two tiers with two translation units, each compiled only with the license its own kernels need: NEON_HP.cpp now builds at +fp16, and the new NEON_FHM.cpp carries the FHM-only entry points at +fp16fml, where they still measure 64 AdvSIMD FMLAL/FMLSL instructions, so the fast path is intact. Dispatch sites gain a second, independently guarded branch (asimdhp && asimdfhm) that tries NEON_FHM first and falls back to the existing asimdhp-only NEON_HP branch.
dor-forer
force-pushed
the
MOD-17845-simd-dispatch-hygiene
branch
from
August 20, 2026 11:11
49f3e8d to
8e71421
Compare
dor-forer
added a commit
that referenced
this pull request
Aug 20, 2026
`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.
2 tasks
dor-forer
force-pushed
the
MOD-17845-simd-dispatch-hygiene
branch
from
August 23, 2026 07:43
8e71421 to
66b6e27
Compare
dor-forer
added a commit
that referenced
this pull request
Aug 23, 2026
`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.
dor-forer
changed the title
dor-forer
force-pushed
the
MOD-17845-simd-dispatch-hygiene
branch
from
August 23, 2026 07:46
66b6e27 to
ee83c94
Compare
dor-forer
added a commit
that referenced
this pull request
Aug 23, 2026
`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.
GuyAv46
previously approved these changes
Aug 23, 2026
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.
dor-forer
dismissed
GuyAv46’s stale review
August 23, 2026 14:16
The merge-base changed after approval.
2 tasks
Each file under spaces/functions/ is compiled for one instruction-set tier under its own -march flags, and the running CPU's feature bits pick which tier's Choose_* entry point is called. Two of those tiers reuse another tier's kernel headers rather than having their own: SVE2.cpp recompiles fourteen of SVE.cpp's headers at -march=armv9-a+sve2, and the NEON_HP and NEON_FHM tiers added earlier in this branch share the two SQ8_FP16 headers at +fp16 and +fp16fml. The kernels are templates at namespace scope with no static, so each instantiation is a weak symbol that both objects define, holding bodies built for different architectures, and the linker keeps whichever it saw first. Nothing in the source decides which. SVE.cpp.o precedes SVE2.cpp.o in the archive, so the SVE bodies win and every Choose_*_SVE2 except the three SQ8_FP16 ones has been dispatching to armv8-a+sve code: the SVE2 tier is selected on SVE2 hardware and runs base-SVE codegen. Measured on Neoverse with gcc 12, the linked FP16_L2Sqr_SVE<false,0> is SVE.cpp.o's 51 instructions rather than SVE2.cpp.o's 49. It is a silent downgrade rather than a fault only because none of the shared bodies currently holds an SVE2-only opcode, and because link order alone picks the winner it can differ between builds of the same commit. The kernels are implementation details of one tier, so this gives them internal linkage: each header opens an anonymous namespace after its own includes and closes it at the end of file. The two tiers may then use identical names while producing independent bodies under their respective flags, and only the Choose_* entry points stay externally visible. Putting the namespace inside the header rather than around the include site is what lets the header keep including its own dependencies, which must stay outside. Covering the header rather than a list of names matters. An earlier attempt renamed each kernel individually and missed SQ8_SQ8_InnerProductSIMD_SVE_IMP, because that list was derived from the symbols nm reported in a release build and -O3 inlines that helper away entirely: 0 symbols in both objects against 8 each for its SQ8_FP32 sibling. It also could not cover the eight step helpers declared plain inline rather than static inline, which collided at -O0 and had no name to rename. The anonymous namespace takes the kernels, the _IMP helpers and the inline helpers alike, so a function added to one of these headers is safe by default. Also adds tests/unit/check_tier_linkage.py, run from ctest as tier_linkage, asserting that no two tier objects in libVectorSimilaritySpaces.a define a symbol in common. It excludes vecsim_types helpers, which come from a shared type header rather than a kernel header and are scalar bit manipulation that every tier compiles to the same bytes, verified byte-identical; float16::cvt is a member function and cannot take internal linkage regardless. Neoverse, gcc 12: release and debug both build clean under -Werror -Wall with no warnings, and tier_linkage passes on both, 8 tier objects over 28 pairs. Spaces suite 1529/1529 on each. The debug run matters because -O0 inlines nothing away, so it is the configuration where a hidden collision would show. On x86_64 the check covers 15 tier objects over 105 pairs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dor-forer
force-pushed
the
MOD-17845-simd-dispatch-hygiene
branch
from
August 24, 2026 10:29
ee83c94 to
03d47a5
Compare
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.
dor-forer
force-pushed
the
MOD-17845-simd-dispatch-hygiene
branch
from
August 24, 2026 10:54
03d47a5 to
809b522
Compare
GuyAv46
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes in the pull request
The
AVX512F_BW_VL_VNNItier is compiled with-mavx512vl, so runtime dispatch must requireavx512vlbefore returning a function from that translation unit. Six of the twelve production predicate sites omitted the check; all twelve now require the complete feature set.The SQ8 unit-test guards now mirror the same predicate, including the dispatcher alignment tests.
Why this is necessary
A translation unit may use every ISA extension enabled by its compiler flags. Selecting it without checking
avx512vlrelies on current processor feature combinations rather than the actual compiled-code contract. No known shipping CPU exposes AVX512 VNNI without VL, so this closes a latent dispatch gap rather than an observed field crash.Which issue this PR fixes
Files modified
src/VecSim/spaces/IP_space.cppsrc/VecSim/spaces/L2_space.cpptests/unit/test_spaces.cppVerification
AVX512F_BW_VL_VNNI.cpptest_spaces: 1584/1584 passedMark if applicable
Note
Low Risk
Tightens CPU-feature checks only; no kernel or API changes. On real CPUs VNNI already implies VL, so this is a latent-dispatch fix rather than a behavior change.
Overview
Runtime dispatch for the
AVX512F_BW_VL_VNNISQ8 kernels now requiresavx512vlin addition to F/BW/VNNI, matching the ISA the kernels are compiled with (-mavx512vl).The missing check is added on SQ8↔FP32 and SQ8↔SQ8 IP, cosine, and L2 choosers in
IP_space.cppandL2_space.cpp. Unit-test feature guards, including alignment-hint tests, use the same predicate so they only exercise that path when VL is present.Reviewed by Cursor Bugbot for commit 809b522. Bugbot is set up for automated code reviews on this repo. Configure here.