Fix: Windows SIMD detection - #5497
Open
Binozo wants to merge 4 commits into
Open
Conversation
Summary: In DD builds SIMDConfig::auto_detect_simd_level() used GNU inline asm (cpuid/xgetbv) guarded by __x86_64__. MSVC defines _M_X64, not __x86_64__, and has no 64-bit GNU-style inline assembly, so the entire detection block compiled to nothing on Windows. Introduce cpuid_count()/xgetbv0() wrappers: __cpuidex/_xgetbv intrinsics under _MSC_VER which also covers clang-cl, existing GNU asm otherwise, and widen the detection guard to __x86_64__ || _M_X64. Remaining detection logic is unchanged.
Summary: TestIndexResidualQuantizerSearch.test_search_L2 asserted inter_ref >= inter_2, i.e. norm-quantized search types can never beat the ST_norm_float reference in recall. That holds only statistically: with shared codebooks and beam search, norm-quantization noise flips near-tie rankings in both directions. The assertion was unreachable on Windows until now: SIMD runtime detection compiled to nothing under MSVC, so only the NONE variant of this test existed there. With AVX2 detection fixed, the AVX2 variant runs on Windows CI for the first time, and MSVC's AVX2 codegen shifts near-ties: inter_2 = 390 vs inter_ref = 387 (3/1000 results). Relax to inter_2 >= inter_ref * 0.95, matching the 5% slack already used for ST_norm_float two lines above and the tie-tolerant bounds in the IVF variant of this test (10-20%).
mnorris11
reviewed
Aug 11, 2026
|
|
||
| namespace { | ||
|
|
||
| #if defined(__x86_64__) || defined(_M_X64) |
Contributor
There was a problem hiding this comment.
IIUC, the minimal solution is to add the _M_X64 and _MSC_VER checks like this line, is that right? If so, should we revert the new functions and regs refactor etc? then it would be easier to merge.
Contributor
Author
There was a problem hiding this comment.
Indeed the _M_X64 and _MSC_VER checks are key but they are not sufficient as widening the guard would not compile under MSVC at all because GNU asm is rejected by MSVC.
Also, cpuid_count has 5 call sites and xgetbv0 has 2 (including detect_x86_uarch_flags which is used in static builds too), so the MSVC intrinsic version would otherwise need to be duplicated inline at 7 sites. So I decided to add a small wrapper to avoid that duplication
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.
Hi,
while testing on windows x64 systems I've noticed that the
ddoptimization compilation target doesn't work properly. Causing crashes on non-avx2 systems for instance.This pr fixes the SIMD level detection.
MSVC doesn't define
__x86_64__but does define_M_X64. This is what this patch is based on.I got some nice GitHub actions and tested the patch on it.
Based on #5476 but reopened because I made a mistake while rebasing