Let the binary Hamming paths reach the VPOPCNT kernels - #5575
Open
mnorris11 wants to merge 4 commits into
Open
Conversation
Contributor
|
@mnorris11 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D118874433. |
Summary:
On aarch64 several faiss dispatch paths silently landed on `SIMDLevel::NONE`
(scalar) or on `ARM_NEON` where an `ARM_SVE` kernel existed. This enforces
SVE -> NEON -> NONE on ARM hosts and fills in the one genuinely missing
kernel pair.
T287037898 reported the scalar-quantizer half: on aarch64 `IndexScalarQuantizer`
with `QT_8bit_direct` / `QT_8bit_direct_signed` cost throughput and bought only
memory, because the `if constexpr` chains in `sq-dispatch.h` enumerated only x86
levels, so ARM fell through to the float-domain `DCTemplate` instead of the
byte-domain integer kernel x86 gets. `Refine(SQ8)` is affected too, since
`IndexRefine::search` goes through `refine_index->get_distance_computer()`.
Three distinct mechanisms, fixed separately:
1. **Missing kernels.** `sq-neon.cpp` had a *scalar* `DistanceComputerByte<Sim,
ARM_NEON>` that was unreachable, and no `DistanceComputerByteSigned<Sim,
ARM_NEON>` at all. Both are now real NEON kernels
(`vabdq_u8`/`vmull_u8`/`vpadalq_u16` for L2 and unsigned IP; `veorq_u8` +
`vmull_s8`/`vpadalq_s16` for the bias-encoded IP). They are bit-identical to
the AVX2 specializations, which the tests rely on. Two invariants worth
naming: the L2 path deliberately keeps an *unsigned* accumulator, because
`vmull_u8` squares reach 255^2 = 65025 and would read as negative in an int16
lane; and `x ^ 0x80` reinterpreted as `int8` is exactly `x - 128`, which is
how the +128 bias comes off before `vmull_s8`.
2. **x86-only `if constexpr` chains.** `ARM_NEON` added to the four chains in
`sq-dispatch.h` (two in `select_distance_computer_body`, two in
`sq_select_InvertedListScanner`), mirroring the already-ARM-inclusive chain
in `is_dimension_compatible`. `ARM_SVE` is deliberately *not* added: the SQ
entry points dispatch with `AVAILABLE_SIMD_LEVELS_A0_SPR`, which has no
`ARM_SVE` bit, so an SVE host already falls through to the `ARM_NEON` case
and now gets these kernels. Adding it would instantiate the *empty* primary
template in `distance_computers.h`, whose stale comment is corrected.
3. **A0 masks hiding existing SVE kernels.** `with_simd_level` uses
`AVAILABLE_SIMD_LEVELS_A0`, which omits `ARM_SVE`, so the DD switch falls
through `case ARM_SVE` to `ARM_NEON`. Switched to `with_simd_level_a1` at the
sites where a real SVE kernel already exists and links: `IndexFlat.cpp` (x4 --
`FlatL2Dis`, `FlatIPDis`, `FlatL2WithNormsDis`, and the base-label search),
`AdditiveQuantizer::compute_centroid_norms`, `SuperKMeans.cpp` (`block_l2`),
all three wrappers in `pq_code_distance-generic.cpp`, and
`with_VectorDistance` in `distances_dispatch.h`. The `IndexFlat` ones matter
most: `faiss::fvec_L2sqr()` already routed A1, so on an SVE host the free
function used SVE while `IndexFlatL2`'s distance computer used NEON.
Also: `FAISS_SIMD_LEVEL=ARM_SVE` on a build without SVE compiled in used to skip
*every* level and run at `NONE`, because `with_selected_simd_levels` has no case
label for an uncompiled level. It now walks down to the nearest compiled level.
The override is still honoured when the *CPU* lacks the level -- forcing a level
is the point of it -- only uncompiled levels are corrected. And the unused
`AVAILABLE_SIMD_LEVELS_A2` (`NONE | AVX2 | ARM_SVE`) is deleted: it is the
NEON-to-NONE trap in constant form, with zero users repo-wide.
Deliberately out of scope, in rough order of remaining value:
- `IndexPQ.cpp:85` / `IndexIVFPQ.cpp:529` still pin PQ to `ARM_NEON` on SVE
hosts. Not a mask fix: `pq_code_distance-sve.cpp` includes only
`pq_scan_impl.h`, not `PQDistanceComputer_impl.h` / `IVFPQScanner_impl.h`, and
`with_HammingComputer<ARM_SVE>` has no complete type at all.
- `distances_aarch64.cpp` -- five `ARM_NEON` specializations
(`fvec_L2sqr_ny{,_nearest,_transposed}`, `fvec_inner_products_ny`) forward to
`<SIMDLevel::NONE>`, so non-SVE aarch64 runs scalar in the IVFFlat scan.
- `rabitq_neon.cpp` -- all six `ARM_NEON` specializations forward to
`<SIMDLevel::NONE>`; there is no SVE variant.
- No `block_l2<ARM_NEON>` and no `exhaustive_L2sqr_blas_cmax<ARM_NEON>`.
- Part two of T287037898 (SDOT/SMMLA). `FEAT_DotProd` / `FEAT_I8MM` are not
expressible as `SIMDLevel`s; the right shape is a runtime `getauxval(AT_HWCAP)`
check inside the ARM TU, following the `SIMDConfig::avx512_split` precedent.
Note the reporter's own caveat that SMMLA measures as *no* improvement until
the scan loop is tiled, so tiling has to be in the same change.
Differential Revision: D118682598
Summary: ## What Split the existing VPOPCNTDQ RaBitQ and Hamming kernels from the full Sapphire Rapids SIMD level. DD builds now expose an `AVX512_VPOPCNT` capability for CPUs such as Ice Lake and Zen 4. The fallback chain is: `AVX512_SPR -> AVX512_VPOPCNT -> AVX512 -> AVX2 -> NONE` The ordinary static AVX512 target remains unchanged and contains no VPOPCNT instructions. ## Why The VPOPCNT kernels only require baseline AVX-512 plus `AVX512_VPOPCNTDQ`. Tying them to the SPR level unnecessarily excluded CPUs that support VPOPCNTDQ but not AVX512-FP16, notably Zen 4. ## Changes made while importing Two changes were needed on top of the pull request. **Feature detection read the wrong CPUID leaf.** `AVX512_VPOPCNTDQ` and `AVX512_VNNI` were read from leaf 1 ECX. Both live in leaf 7 subleaf 0 ECX, at bits 14 and 11. Leaf 1 ECX holds unrelated bits at those positions, so the new level was never selected on any CPU. Measured on an AMD Genoa host before the fix: the level reported as unavailable and detection returned `AVX512`. After the fix the same host reports `AVX512_VPOPCNT`. **The build definitions outside CMake were not updated.** The pull request renames three files. The other build description still named the old paths and had no entry for the new level, so the library did not build. This adds the level constant, its compiler flags, its `COMPILE_SIMD_AVX512_VPOPCNT` define, and the renamed paths. The mask and helper names also follow the naming used in the parent diff, so `AVAILABLE_SIMD_LEVELS_BASE_WITH_VPOPCNT` and `with_simd_level_with_vpopcnt`. Pull Request resolved: facebookresearch#5531 Test Plan: ``` buck2 test fbcode//mode/opt -c faiss.dynamic_dispatch=true \ fbcode//faiss/tests:test_simd_levels fbcode//faiss/tests:test_rabitq_simd ``` Result: all pass. The build failures the imported version reported are cleared. All three configurations build: ``` buck2 build fbcode//mode/dev fbcode//faiss:faiss fbcode//faiss:faiss_no_multithreading fbcode//faiss:faiss_omp_mock buck2 build fbcode//mode/opt -c faiss.dynamic_dispatch=true <same three targets> buck2 build fbcode//mode/dev -c fbcode.arch=aarch64 <same three targets> ``` Detection on an AMD Genoa host, which has AVX512_VPOPCNTDQ and AVX512_BF16 but not AVX512_FP16: | | before the CPUID fix | after | | --- | --- | --- | | detected level | `AVX512` | `AVX512_VPOPCNT` | | `AVX512_VPOPCNT` available | 0 | 1 | | `AVX512_SPR` available | 0 | 0 | Differential Revision: D118969754 Pulled By: mnorris11
Summary: `IVFBinaryScannerL2::scan_codes` reads `simi[0]` to find the heap top. A k of 0 leaves no heap to read, so the scan read past the end of an empty array. This guards that case and returns 0. The change also records how a caller bounds a top-k scan by a radius, because nothing said so and the answer is not obvious from the code. The heap top is the only bound the scan applies. A caller that wants the k nearest codes inside a radius therefore seeds every heap slot with that radius, in place of the neutral value that `heap_heapify` writes. The scan then rejects any code at or beyond the radius. A slot the scan never fills keeps its label of -1, which is how the caller tells a result from an empty slot. That idiom needs no new state on the scanner, and it keeps one rule in the loop: a code must beat the heap top. A second bound would make the loop harder to reason about, and a radius has no meaning for a caller that only wants the k nearest codes. The loop now holds the heap top in a local. Only an accepted code can lower it, so the loop reads it again inside the branch rather than on every iteration. On a scan that accepts few codes, that turns a load into a register read for almost every code. Differential Revision: D118925150
Summary: Two things kept the binary Hamming paths on scalar popcount, whatever the host. `IndexBinaryIVF` builds its scanner and runs its searches through `with_simd_level`, which uses the BASE level mask. That mask holds no `AVX512_VPOPCNT` bit, so on a host that reports that level the dispatch falls through to `AVX512`, where the wide computers use scalar popcount. Two build variants, `faiss_omp_mock` and `faiss_no_multithreading`, hardcoded dynamic dispatch off. They compiled the AVX2 kernels only, so a consumer of either variant could not use runtime dispatch at all, whatever the build setting said. This change: - Moves the three dispatch sites in `IndexBinaryIVF.cpp` to `with_simd_level_with_vpopcnt`. Below that level the dispatch falls through to `AVX512`, as before. - Instantiates `IndexBinaryIVF_impl.h` in `hamming_avx512_vpopcnt.cpp`. That is the one translation unit compiled with `-mavx512vpopcntdq`, which the VPOPCNT computers need. Without it the new dispatch has no symbol to call. The include must follow the computer specializations, so it is placed after them. - Lets both build variants honour the dynamic dispatch setting. `HammingComputer16` and `HammingComputer20` still inherit the scalar body at this level, so a 16-byte or 20-byte code gains nothing here. Differential Revision: D118874433
meta-codesync
Bot
changed the title
mnorris11
force-pushed
the
export-D118874433
branch
from
September 6, 2026 04:12
a18b67c to
72ae743
Compare
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.
Summary:
Two things kept the binary Hamming paths on scalar popcount, whatever the host.
IndexBinaryIVFbuilds its scanner and runs its searches throughwith_simd_level, which uses the BASE level mask. That mask holds noAVX512_VPOPCNTbit, so on a host that reports that level the dispatch falls through toAVX512, where the wide computers use scalar popcount.Two build variants,
faiss_omp_mockandfaiss_no_multithreading, hardcoded dynamic dispatch off. They compiled the AVX2 kernels only, so a consumer of either variant could not use runtime dispatch at all, whatever the build setting said.This change:
IndexBinaryIVF.cpptowith_simd_level_with_vpopcnt. Below that level the dispatch falls through toAVX512, as before.IndexBinaryIVF_impl.hinhamming_avx512_vpopcnt.cpp. That is the one translation unit compiled with-mavx512vpopcntdq, which the VPOPCNT computers need. Without it the new dispatch has no symbol to call. The include must follow the computer specializations, so it is placed after them.HammingComputer16andHammingComputer20still inherit the scalar body at this level, so a 16-byte or 20-byte code gains nothing here.Differential Revision: D118874433