[MOD-17844] Stop ARM SIMD tiers executing instructions the running CPU lacks - #1018
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
========================================
Coverage 97.18% 97.18%
========================================
Files 141 141
Lines 8420 8537 +117
========================================
+ Hits 8183 8297 +114
- Misses 237 240 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dor-forer
force-pushed
the
MOD-17844-arm-simd-isa-safety
branch
from
August 20, 2026 08:35
f527661 to
f2757e4
Compare
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-17844-arm-simd-isa-safety
branch
from
August 20, 2026 11:11
f2757e4 to
0d42fb4
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
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
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
reviewed
Aug 23, 2026
Comment on lines
+11
to
+23
| // Hoisted above the anonymous namespace below so that the standard library and the shared | ||
| // type headers keep external linkage. Wrapping them would pull <cstring> 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 <arm_neon.h> | ||
|
|
||
| // 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. |
Collaborator
There was a problem hiding this comment.
Consider renaming the problematic functions instead of adding these includes everywhere
Collaborator
Author
There was a problem hiding this comment.
You were right, and more right than I realised. Done in 36042c9, which drops the linkage commit entirely and replaces it with renaming.
Measuring which tier pairs actually collide, on this PR's base commit:
| pair | shared external symbols |
|---|---|
SVE.o / SVE2.o |
152 |
| the other 27 ARM pairs | 0 |
| all 105 x86 pairs | 0 |
So the change to NEON.cpp you were reading, and the ones to NEON_BF16.cpp, NEON_DOTPROD.cpp, NEON_HP.cpp, NEON_FHM.cpp and SVE_BF16.cpp, fixed nothing. Six of the eight TUs I touched had no collision to fix. NEON.cpp was a reasonable place to ask.
I should also correct the description I had written: it claimed NEON.o and NEON_DOTPROD.o shared 93 symbols. They share 0, and not because main moved under the branch. #1014 and #1015 are both ancestors of this PR's base 7a5fe7f9, so the renames they did had already removed that collision before I opened this. The number was stale when I wrote it. Scenario 3 in the description depended on it and is gone too.
On renaming: the leaf kernels already carry their ISA (INT8_InnerProductSIMD16_NEON vs _NEON_DOTPROD), so there was nothing misnamed to fix. The one real collision is that SVE2.cpp includes fourteen of SVE.cpp's headers on purpose and recompiles them at armv9-a+sve2, so it is one source text producing two different bodies under one name. The rename has to happen at the include site, which is 19 #defines at the top of SVE2.cpp:
#define FP32_InnerProductSIMD_SVE FP32_InnerProductSIMD_SVE2
#define FP32_L2SqrSIMD_SVE FP32_L2SqrSIMD_SVE2
...Each one covers both the definition in the header and the use in the Choose_* body below, since those reference the same names. 1 file, +26/-15, instead of 8 files and +190.
Your suggestion also turned out to fix something the anonymous namespace did not make visible. SVE.cpp.o precedes SVE2.cpp.o in the archive and first-in-link-order wins, so on main today every Choose_*_SVE2 except the three SQ8_FP16 ones is dispatching to armv8-a+sve bodies. The SVE2 tier is selected on SVE2 hardware and runs base-SVE codegen. The linked FP16_L2Sqr_SVE<false,0> is SVE.cpp.o's 51 instructions, not SVE2.cpp.o's 49. Internal linkage would have kept the right body but left the name lying about which -march built it; renaming makes it say so.
And the best argument for your approach was already in this PR. NEON_HP.cpp and NEON_FHM.cpp include the same two SQ8_FP16 headers at +fp16 and +fp16fml, which is structurally the identical hazard, and they share 0 symbols with no linkage tricks at all, purely because the kernels inside are named _NEON_HP and _NEON_FHM. Defect 1's own fix already used naming. SVE2.cpp was the only file not following the convention.
The one thing the boilerplate did buy was covering headers nobody had thought about yet, so I replaced that with a check instead: tests/unit/check_tier_linkage.py, run from ctest as tier_linkage. It nms the archive and fails if any two tier objects define a symbol in common. Architecture-agnostic, passes on x86 (15 objects, 105 pairs, 0 shared), and if I strip the renames back out it fails with all 152 symbols named. So the invariant is enforced rather than remembered, and it protects the NEON tiers without putting anything in them.
Neoverse, gcc 12: full build clean, 1529/1529.
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
force-pushed
the
MOD-17844-arm-simd-isa-safety
branch
from
August 23, 2026 14:16
0d42fb4 to
36042c9
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 36042c9. Configure here.
2 tasks
dor-forer
force-pushed
the
MOD-17844-arm-simd-isa-safety
branch
from
August 24, 2026 06:33
60c9c60 to
cbe416d
Compare
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-17844-arm-simd-isa-safety
branch
2 times, most recently
from
August 24, 2026 10:54
79905e8 to
3d3fb2e
Compare
dor-forer
force-pushed
the
MOD-17844-arm-simd-isa-safety
branch
from
August 24, 2026 10:54
3d3fb2e to
2fc6c1c
Compare
GuyAv46
approved these changes
Aug 25, 2026
| if(PYTHON3_FOR_TESTS) | ||
| add_test(NAME tier_linkage | ||
| COMMAND ${PYTHON3_FOR_TESTS} ${CMAKE_CURRENT_SOURCE_DIR}/check_tier_linkage.py | ||
| $<TARGET_FILE:VectorSimilaritySpaces>) |
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
Two defects that let an ARM SIMD tier execute instructions the running CPU does not have. Both are correctness fixes, not performance work: see the benchmark section below, which finds no throughput change outside the paths that were already correct.
functions/NEON_HP.cppwas one translation unit compiled with-march=armv8.2-a+fp16fml, holding both the HP-only and the FHM entry points. The whole TU therefore carried the fp16fml licence, and the compiler contracted the HP path'svcvt_f32_f16plusvfmaq_f32intofmlal, even though the HP source uses no FMLAL intrinsic. That path is dispatched onfeatures.asimdhpalone, so on a core withasimdhpbut withoutasimdfhmit is a SIGILL, reachable from any SQ8_FP16 IP, L2 or Cosine query. Measured on Neoverse with gcc 12: 32 AdvSIMD FMLAL instructions in the HP-only wrappers at+fp16fml, 0 at+fp16, 96 across the whole TU.Fixed by splitting into two tiers:
NEON_HPat+fp16predicated onasimdhp, and a newNEON_FHMat+fp16fmlpredicated onasimdhp && asimdfhm. Tightening the predicate to require both bits instead would have deleted the HP-only fallback for exactly the CPUs that need it. The FHM tier still emits 64 FMLAL instructions, so its fast path is unchanged.SVE2.cpprecompiles fourteen ofSVE.cpp's kernel headers at-march=armv9-a+sve2rather than-march=armv8-a+sve. The kernels are templates at namespace scope with nostatic, 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.Fixed by giving those definitions internal linkage: each of the fourteen headers
SVE2.cppshares withSVE.cppopens an anonymous namespace after its own includes and closes it at end of file. The tiers may then use identical names while producing independent bodies under their respective flags, and only theChoose_*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 it.SVE2.cppitself is unchanged.The
NEON_HP/NEON_FHMpair is deliberately left alone. It shares the two SQ8_FP16 headers at different-march, so it is the same structural hazard, but the kernels inside already carry_NEON_HPand_NEON_FHMin their names and the pair shares 0 symbols in Release and Debug alike. Wrapping those headers would have fixed nothing measured, so that pair relies on the naming convention, withtier_linkageguarding it.Defect 1 is reproduced, not just inferred. On an AWS Graviton2 (Neoverse-N1,
asimdhppresent andasimdfhmabsent, gcc 12), a program that callsL2_SQ8_FP16_GetDistFunc/IP_.../Cosine_...witharch_opt = nullptr, so the CPU's own feature bits choose the tier, dies withIllegal instruction (core dumped), exit 132, on the first call. On this branch the same binary returns from all three. The tier objects on that same box:NEON_HP.cpp.ocarries 96fmlalon main and 0 here, with the 64 in the newNEON_FHM.cpp.oreachable only whenasimdfhmis set.Scope of defect 2, measured
SVE.cpp.oandSVE2.cpp.oshare 152 externally defined symbols on this PR's base commit. Every one of the other 27 ARM tier pairs shares 0, and so do all 105 x86 tier pairs, because those kernel names embed the ISA.SVE.cpp.oprecedesSVE2.cpp.oin the archive and first-in-link-order wins, so the SVE bodies survive and everyChoose_*_SVE2except the three SQ8_FP16 ones has been dispatching toarmv8-a+svecode: the SVE2 tier is selected on SVE2 hardware and runs base-SVE codegen. Confirmed by linking a probe, where the survivingFP16_L2Sqr_SVE<false,0>isSVE.cpp.o's 51 instructions rather thanSVE2.cpp.o's 49.It is a silent downgrade rather than a fault only because none of the 152 bodies currently holds an SVE2-only opcode: all 152 differ between the two objects, but diffing the instruction mnemonics of every shared body gives the empty set in both directions, so the difference is scheduling and register allocation. It becomes a SIGILL on an SVE-without-SVE2 core the first time the compiler emits one, and because link order alone picks the winner it can differ between builds of the same commit.
Why the namespace rather than renaming each kernel
An earlier revision of this PR renamed the kernels individually with
#definealiases inSVE2.cpp. That approach is incomplete by construction, and was: it missedSQ8_SQ8_InnerProductSIMD_SVE_IMP, because the list was derived from the symbolsnmreported in a release build and-O3inlines 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 plaininlinerather thanstatic inline, which collided at-O0and had no name to alias. The anonymous namespace covers the kernels, the_IMPhelpers and theinlinehelpers alike, so a function added to one of these headers is safe by default.Regression test
tests/unit/check_tier_linkage.py, run from ctest astier_linkage,nmslibVectorSimilaritySpaces.aand asserts that no two tier objects define a symbol in common. It is architecture-agnostic and covers x86 as well, 15 tier objects over 105 pairs. It excludesvecsim_typeshelpers, 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::cvtis a member function and cannot take internal linkage regardless.The check is run on Debug as well as Release, which matters: at
-O0nothing is inlined away, so it is the configuration where a hidden collision shows. The alias revision above could never pass at-O0; this one does.Benchmark: the shared SVE2 kernels do not earn their recompilation
bm-spacesonr8g.xlarge(Neoverse-V2, real SVE2), 558 SVE vs SVE2 pairs. These numbers are only meaningful on this branch, because on main the SVE2 choosers resolve toSVE.cpp.o's bodies and the comparison would measure SVE against itself.+sve2Per-type medians for the shared kernels: FP16 1.000, FP32 0.998, FP64 0.994, SQ8_FP32 0.982, SQ8_SQ8 1.000. That is noise, and it matches the codegen: identical instruction vocabulary, plus or minus 16 instructions, no SVE2-only opcodes.
The 1.83x on SQ8_FP16 is not gained by this PR. Those kernels live in
IP/L2_SVE2_SQ8_FP16.h, are already named_SVE2, never collided, and already work on main.Follow-up, not done here: the eighteen shared choosers could delegate to
Choose_*_SVEinstead of recompiling, which would delete a tier's worth of duplicate object code that buys nothing measurable. That touches roughly 250 reference sites across 13 files, mostlytest_spaces.cppand the benchmark registrations, so it belongs in its own change.Validation
Neoverse-N1 (gcc 12), this commit: Release and Debug both build clean under
-Werror -Wallwith zero warnings,tier_linkagepasses on both over 8 tier objects and 28 pairs, and the spaces suite is 1529/1529 on each. The Debug run is the meaningful one for defect 2, since-O0inlines nothing away. That box hasasimdhpwithoutasimdfhm, which is what makes it the right hardware for the defect 1 repro, but it has no SVE at all, so it does not execute the SVE or SVE2 kernels.Real SVE2 coverage came from a
workflow_dispatchofarm.yml, which provisionsr8g.xlarge: 2704/2704, withsve sve2 svebf16 i8mmin the runner'slscpu, so theif (optimization.sve2)assertion paths executed rather than being skipped for a missing feature bit. That run covered an earlier revision of this branch, which reached the same end state by renaming the kernels instead of scoping them; the kernel bodies it exercised are identical to the ones here, but it has not been repeated against this commit. CI's own ARM job runs at merge-queue time.The
tier_linkagecheck also passes against an x86_64 Release archive, 15 tier objects over 105 pairs.Which issues this PR fixes
Main objects this PR modified
src/VecSim/spaces/functions/NEON_HP.{cpp,h}and newNEON_FHM.{cpp,h}src/VecSim/spaces/{IP,L2}_space.cppdispatch sites for SQ8_FP16cmake/aarch64InstructionFlags.cmakeandsrc/VecSim/spaces/CMakeLists.txtfor the new tiersrc/VecSim/spaces/{IP,L2}/thatSVE.cppandSVE2.cppsharetests/unit/check_tier_linkage.py,tests/unit/CMakeLists.txt,tests/unit/test_spaces.cppandtests/benchmark/spaces_benchmarks/Mark if applicable
Note
High Risk
Changes ARM SIMD compilation and runtime dispatch for vector distance kernels. A mistake here can SIGILL or silently run the wrong ISA on the query path.
Overview
Stops ARM distance kernels from running instructions the CPU does not have.
NEON HP vs FHM:
NEON_HP.cppwas compiled with+fp16fmleven for HP-only SQ8↔FP16 paths, so the compiler could emitfmlalon cores withasimdhpbut notasimdfhm(SIGILL). HP now builds with+fp16and a newNEON_FHMTU builds with+fp16fml. Dispatch requiresasimdhp && asimdfhmfor FHM, then falls back to HP onasimdhpalone.SVE vs SVE2: Shared kernel headers were compiled into both TUs as weak namespace-scope templates, so link order picked one
-marchfor both choosers. Those headers now wrap kernels in an anonymous namespace so each tier keeps its own bodies; onlyChoose_*stays external.Adds
tier_linkage(check_tier_linkage.py) to fail if any two SIMD tier objects export the same symbol.Reviewed by Cursor Bugbot for commit 2fc6c1c. Bugbot is set up for automated code reviews on this repo. Configure here.