feat: add Windows ARM64 (MSVC) build support - #352
Conversation
|
hi, @Aanerud, it would be beneficial to introduce a CI workflow for Windows ARM64 to guarantee ongoing integration support. |
Extends the existing Windows job in 05-windows-build.yml to also cover `windows-11-arm` so the MSVC ARM64 build path is exercised on every PR, per request from @feihongxu0824 on alibaba#352. Changes: * Add a third row to the matrix with `platform: windows-11-arm`, `msvc_arch: arm64`, and `python_version: '3.11'` (Python 3.10 has no official Windows-on-ARM installer; 3.11 is the first). * Parameterize the existing `ilammy/msvc-dev-cmd@v1` step on `matrix.msvc_arch` instead of hard-coded `x64`, and the `actions/setup-python@v6` step on `matrix.python_version`. No changes to the x64 rows (still Python 3.10 + MSVC x64) and no changes to the build/test steps themselves — same `pip install -v .`, same C++ unittest run, same pytest, same examples. `fail-fast: false` was already set so an ARM64 regression will not hide x64 regressions and vice versa.
|
Thanks for the review @feihongxu0824 — CLA signed, and just pushed Kept it minimal to match the existing style:
Happy to split the ARM64 row into its own reusable workflow file (e.g. |
Extends the existing Windows job in 05-windows-build.yml to also cover `windows-11-arm` so the MSVC ARM64 build path is exercised on every PR, per request from @feihongxu0824 on alibaba#352. Changes: * Add a third row to the matrix with `platform: windows-11-arm`, `msvc_arch: arm64`, and `python_version: '3.11'` (Python 3.10 has no official Windows-on-ARM installer; 3.11 is the first). * Parameterize the existing `ilammy/msvc-dev-cmd@v1` step on `matrix.msvc_arch` instead of hard-coded `x64`, and the `actions/setup-python@v6` step on `matrix.python_version`. No changes to the x64 rows (still Python 3.10 + MSVC x64) and no changes to the build/test steps themselves — same `pip install -v .`, same C++ unittest run, same pytest, same examples. `fail-fast: false` was already set so an ARM64 regression will not hide x64 regressions and vice versa.
3f586d1 to
3350494
Compare
|
The
Both asserts are Root cause is test sensitivity, not an index bug. The dataset vectors are constructed with small inter-vector deltas ( Signal pattern that makes me confident in this diagnosis:
Just pushed If you'd rather I do the MSVC-ARM64 NEON work in this PR instead of skipping + follow-up, I'm happy to — let me know. |
|
Apologies for the delay in reviewing this PR. Thank you for your contribution and for providing such a clear description. I’ve submitted some suggestions for your reference. Also, I noticed that the CLA hasn’t been signed yet. |
3d13553 to
aa76ffa
Compare
Extends the existing Windows job in 05-windows-build.yml to also cover `windows-11-arm` so the MSVC ARM64 build path is exercised on every PR, per request from @feihongxu0824 on alibaba#352. Changes: * Add a third row to the matrix with `platform: windows-11-arm`, `msvc_arch: arm64`, and `python_version: '3.11'` (Python 3.10 has no official Windows-on-ARM installer; 3.11 is the first). * Parameterize the existing `ilammy/msvc-dev-cmd@v1` step on `matrix.msvc_arch` instead of hard-coded `x64`, and the `actions/setup-python@v6` step on `matrix.python_version`. No changes to the x64 rows (still Python 3.10 + MSVC x64) and no changes to the build/test steps themselves — same `pip install -v .`, same C++ unittest run, same pytest, same examples. `fail-fast: false` was already set so an ARM64 regression will not hide x64 regressions and vice versa.
Addresses @feihongxu0824's review comment on alibaba#352. Previously the *_neon.cc files compiled into empty translation units on MSVC ARM64 because their guards (`__ARM_NEON`, `__aarch64__`) are GCC/Clang-only macros, leaving zvec to use the scalar fallback. That fallback hits ~1 ULP precision drift versus the NEON path, which surfaced as two HnswStreamerTest cosine failures and a `NormMatrix.Norm1_General` failure on the `windows-11-arm` CI runner. Changes: * Expand `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) || defined(_M_ARM64))` at the 51 source sites that gate ARM NEON kernels (math, math_batch, utility, normalizer, platform headers, dispatch tables, version probe). * Expand `defined(__aarch64__)` -> `(defined(__aarch64__) || defined(_M_ARM64))` at the 26 sites that distinguish AArch64 from ARMv7 NEON — MSVC ARM64 is AArch64 but does not predefine `__aarch64__`. As a side effect the ARMv7-only polyfills (`vaddvq_f32`/`vaddvq_s32` shims in `distance_matrix_accum_fp32.i`, `distance_matrix_fp32.i`) are correctly skipped under MSVC ARM64, where those intrinsics are built in. * `src/include/zvec/ailego/internal/platform.h`: include `<arm_neon.h>` on the MSVC branch when `_M_ARM64` is defined (it was previously gated behind `!_MSC_VER`, so MSVC ARM64 saw no NEON types). * `src/ailego/CMakeLists.txt`: keep the existing `if(NOT MSVC)` wrapper around the GCC-only `-march=armv8-a` flag, and add an explicit `else()` branch with a comment explaining MSVC ARM64 does not need `-march` (NEON is the ARMv8 baseline on MSVC and the kernels are now picked up via the ALL_SRCS glob with the macro guards above). This supersedes the earlier `test(hnsw): skip two cosine self-match tests on MSVC ARM64` commit (which has been dropped from the branch). The NEON math kernels now use the same precision path as Linux/macOS ARM64, so those tests should pass natively on `windows-11-arm`.
|
Rebased onto current
For commit 3, instead of leaving the kernels gated on the GCC/Clang-only macros, I expanded the source guards as you suggested:
One subtlety: I also dropped the old The CLA-bot pending status was because my earlier commits were authored as Waiting on the |
Addresses @feihongxu0824's review comment on alibaba#352. Previously the *_neon.cc files compiled into empty translation units on MSVC ARM64 because their guards (`__ARM_NEON`, `__aarch64__`) are GCC/Clang-only macros, leaving zvec to use the scalar fallback. That fallback hits ~1 ULP precision drift versus the NEON path, which surfaced as two HnswStreamerTest cosine failures and a `NormMatrix.Norm1_General` failure on the `windows-11-arm` CI runner. Changes: * Expand `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) || defined(_M_ARM64))` at the 51 source sites that gate ARM NEON kernels (math, math_batch, utility, normalizer, platform headers, dispatch tables, version probe). * Expand `defined(__aarch64__)` -> `(defined(__aarch64__) || defined(_M_ARM64))` at the 26 sites that distinguish AArch64 from ARMv7 NEON — MSVC ARM64 is AArch64 but does not predefine `__aarch64__`. As a side effect the ARMv7-only polyfills (`vaddvq_f32`/`vaddvq_s32` shims in `distance_matrix_accum_fp32.i`, `distance_matrix_fp32.i`) are correctly skipped under MSVC ARM64, where those intrinsics are built in. * `src/include/zvec/ailego/internal/platform.h`: include `<arm_neon.h>` on the MSVC branch when `_M_ARM64` is defined (it was previously gated behind `!_MSC_VER`, so MSVC ARM64 saw no NEON types). * `src/ailego/CMakeLists.txt`: keep the existing `if(NOT MSVC)` wrapper around the GCC-only `-march=armv8-a` flag, and add an explicit `else()` branch with a comment explaining MSVC ARM64 does not need `-march` (NEON is the ARMv8 baseline on MSVC and the kernels are now picked up via the ALL_SRCS glob with the macro guards above). This supersedes the earlier `test(hnsw): skip two cosine self-match tests on MSVC ARM64` commit (which has been dropped from the branch). The NEON math kernels now use the same precision path as Linux/macOS ARM64, so those tests should pass natively on `windows-11-arm`.
aa76ffa to
0dc8e61
Compare
Addresses @feihongxu0824's review comment on alibaba#352. Previously the *_neon.cc files compiled into empty translation units on MSVC ARM64 because their guards (`__ARM_NEON`, `__aarch64__`) are GCC/Clang-only macros, leaving zvec to use the scalar fallback. That fallback hits ~1 ULP precision drift versus the NEON path, which surfaced as two HnswStreamerTest cosine failures and a `NormMatrix.Norm1_General` failure on the `windows-11-arm` CI runner. Changes: * Expand `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) || defined(_M_ARM64))` at the 51 source sites that gate ARM NEON kernels (math, math_batch, utility, normalizer, platform headers, dispatch tables, version probe). * Expand `defined(__aarch64__)` -> `(defined(__aarch64__) || defined(_M_ARM64))` at the 26 sites that distinguish AArch64 from ARMv7 NEON — MSVC ARM64 is AArch64 but does not predefine `__aarch64__`. As a side effect the ARMv7-only polyfills (`vaddvq_f32`/`vaddvq_s32` shims in `distance_matrix_accum_fp32.i`, `distance_matrix_fp32.i`) are correctly skipped under MSVC ARM64, where those intrinsics are built in. * `src/include/zvec/ailego/internal/platform.h`: include `<arm_neon.h>` on the MSVC branch when `_M_ARM64` is defined (it was previously gated behind `!_MSC_VER`, so MSVC ARM64 saw no NEON types). * `src/ailego/CMakeLists.txt`: keep the existing `if(NOT MSVC)` wrapper around the GCC-only `-march=armv8-a` flag, and add an explicit `else()` branch with a comment explaining MSVC ARM64 does not need `-march` (NEON is the ARMv8 baseline on MSVC and the kernels are now picked up via the ALL_SRCS glob with the macro guards above). This supersedes the earlier `test(hnsw): skip two cosine self-match tests on MSVC ARM64` commit (which has been dropped from the branch). The NEON math kernels now use the same precision path as Linux/macOS ARM64, so those tests should pass natively on `windows-11-arm`.
0dc8e61 to
afb6654
Compare
Addresses @feihongxu0824's review comment on alibaba#352. Previously the *_neon.cc files compiled into empty translation units on MSVC ARM64 because their guards (`__ARM_NEON`, `__aarch64__`) are GCC/Clang-only macros, leaving zvec to use the scalar fallback. That fallback hits ~1 ULP precision drift versus the NEON path, which surfaced as two HnswStreamerTest cosine failures and a `NormMatrix.Norm1_General` failure on the `windows-11-arm` CI runner. Changes: * Expand `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) || defined(_M_ARM64))` at the 51 source sites that gate ARM NEON kernels (math, math_batch, utility, normalizer, platform headers, dispatch tables, version probe). * Expand `defined(__aarch64__)` -> `(defined(__aarch64__) || defined(_M_ARM64))` at the 26 sites that distinguish AArch64 from ARMv7 NEON — MSVC ARM64 is AArch64 but does not predefine `__aarch64__`. As a side effect the ARMv7-only polyfills (`vaddvq_f32`/`vaddvq_s32` shims in `distance_matrix_accum_fp32.i`, `distance_matrix_fp32.i`) are correctly skipped under MSVC ARM64, where those intrinsics are built in. * `src/include/zvec/ailego/internal/platform.h`: include `<arm_neon.h>` on the MSVC branch when `_M_ARM64` is defined (it was previously gated behind `!_MSC_VER`, so MSVC ARM64 saw no NEON types). * `src/ailego/CMakeLists.txt`: keep the existing `if(NOT MSVC)` wrapper around the GCC-only `-march=armv8-a` flag, and add an explicit `else()` branch with a comment explaining MSVC ARM64 does not need `-march` (NEON is the ARMv8 baseline on MSVC and the kernels are now picked up via the ALL_SRCS glob with the macro guards above). This supersedes the earlier `test(hnsw): skip two cosine self-match tests on MSVC ARM64` commit (which has been dropped from the branch). The NEON math kernels now use the same precision path as Linux/macOS ARM64, so those tests should pass natively on `windows-11-arm`.
Addresses @feihongxu0824's review comment on alibaba#352. Previously the *_neon.cc files compiled into empty translation units on MSVC ARM64 because their guards (`__ARM_NEON`, `__aarch64__`) are GCC/Clang-only macros, leaving zvec to use the scalar fallback. That fallback hits ~1 ULP precision drift versus the NEON path, which surfaced as two HnswStreamerTest cosine failures and a `NormMatrix.Norm1_General` failure on the `windows-11-arm` CI runner. Changes: * Expand `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) || defined(_M_ARM64))` at the 51 source sites that gate ARM NEON kernels (math, math_batch, utility, normalizer, platform headers, dispatch tables, version probe). * Expand `defined(__aarch64__)` -> `(defined(__aarch64__) || defined(_M_ARM64))` at the 26 sites that distinguish AArch64 from ARMv7 NEON — MSVC ARM64 is AArch64 but does not predefine `__aarch64__`. As a side effect the ARMv7-only polyfills (`vaddvq_f32`/`vaddvq_s32` shims in `distance_matrix_accum_fp32.i`, `distance_matrix_fp32.i`) are correctly skipped under MSVC ARM64, where those intrinsics are built in. * `src/include/zvec/ailego/internal/platform.h`: include `<arm_neon.h>` on the MSVC branch when `_M_ARM64` is defined (it was previously gated behind `!_MSC_VER`, so MSVC ARM64 saw no NEON types). * `src/ailego/CMakeLists.txt`: keep the existing `if(NOT MSVC)` wrapper around the GCC-only `-march=armv8-a` flag, and add an explicit `else()` branch with a comment explaining MSVC ARM64 does not need `-march` (NEON is the ARMv8 baseline on MSVC and the kernels are now picked up via the ALL_SRCS glob with the macro guards above). This supersedes the earlier `test(hnsw): skip two cosine self-match tests on MSVC ARM64` commit (which has been dropped from the branch). The NEON math kernels now use the same precision path as Linux/macOS ARM64, so those tests should pass natively on `windows-11-arm`.
Addresses review feedback that the inline _M_ARM64 preprocessor guards
were confusing and needed simplifying. The NEON-enablement change had
mechanically expanded `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) ||
defined(_M_ARM64))` and `defined(__aarch64__)` -> `(defined(__aarch64__)
|| defined(_M_ARM64))` at every site, producing long, hard-to-read compound
expressions -- one of which (`(NEON||M) && !(aarch64||M)`) was even
logically dead on MSVC ARM64.
Introduce two self-documenting feature macros in platform.h, defined once:
AILEGO_ARM64 - 64-bit ARM (AArch64): __aarch64__ (GCC/Clang) or
_M_ARM64 (MSVC)
AILEGO_HAVE_NEON - NEON intrinsics available: __ARM_NEON (GCC/Clang) or
_M_ARM64 (MSVC ARM64, ARMv8 baseline)
and replace the verbose guards across the ailego math kernels, platform.h,
cpu_features.cc and bitset_helper.cc with them. This is a pure readability
change -- every guard expands to the same truth value on every target as
before, so behaviour is unchanged. cpu_features.cc now includes platform.h
so it can use the macros.
Verified on Windows ARM64 (MSVC): zvec_ailego compiles, and a preprocessor
probe confirms both macros resolve true so the NEON kernels are still
compiled in (not the scalar fallback).
bb076a7 to
333ac6e
Compare
|
Thanks for the review @JalinWang. Addressed the What changed The NEON-enablement commit had mechanically expanded the guards (
All ~25 guard sites across the ailego math kernels,
It's a pure readability change — every guard expands to the same truth value on every target as before. Verification (local, Windows ARM64 / MSVC)
All 4 commits are authored with my CLA-signed email. Waiting on the |
There was a problem hiding this comment.
Pull request overview
This PR extends zvec’s build system to support producing native Windows ARM64 Python wheels with MSVC/VS 2022, including targeted architecture-guard fixes in ailego, Arrow configuration changes for ARM64+MSVC, and CI matrix expansion for Windows ARM64.
Changes:
- Introduces centralized ARM64/NEON feature macros (
AILEGO_ARM64,AILEGO_HAVE_NEON) and updates ailego NEON guards to work on MSVC ARM64. - Adjusts build configuration for Windows ARM64: fixes ailego CMake early-return, disables Arrow SIMD only on ARM64, and applies an ARM64-specific Arrow patch.
- Updates the Windows GitHub Actions workflow to include an ARM64 job (Python 3.11, MSVC arm64).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
thirdparty/FastPFOR/CMakeLists.txt |
Marks SIMDe include dir as SYSTEM for consumers to avoid MSVC ARM64 warnings-as-errors. |
thirdparty/arrow/CMakeLists.txt |
Applies an ARM64 MSVC patch and disables Arrow SIMD only on ARM64 MSVC. |
thirdparty/arrow/arrow.windows-arm64.patch |
Patches Arrow’s vendored PCG uint128 implementation for MSVC ARM/ARM64 (endianness + mul intrinsic). |
src/include/zvec/ailego/utility/float_helper.h |
Ensures MSVC ARM64 uses the uint16 storage path (MSVC lacks __fp16). |
src/include/zvec/ailego/internal/platform.h |
Adds unified ARM64/NEON feature macros and MSVC ARM64 NEON header inclusion. |
src/include/zvec/ailego/buffer/concurrentqueue.h |
Adjusts thread-local guards to account for MSVC ARM64 macro definitions. |
src/ailego/version.i |
Switches SIMD version reporting to the new AILEGO_HAVE_NEON macro. |
src/ailego/utility/float_helper.cc |
Avoids __fp16 path on MSVC ARM64 by tightening the __aarch64__ guard. |
src/ailego/utility/bitset_helper.cc |
Uses the new NEON/ARM64 macros for NEON code paths. |
src/ailego/math/normalizer.h |
Updates NEON gating for FP32 normalization via AILEGO_* macros and documents FP16 constraints. |
src/ailego/math/normalizer.cc |
Updates NEON gating and excludes FP16 NEON implementations on MSVC ARM64. |
src/ailego/math/norm2_matrix.h |
Updates NEON/ARM64 guards for norm2 matrix specializations. |
src/ailego/math/norm2_matrix_fp32.cc |
Updates runtime dispatch to use AILEGO_HAVE_NEON. |
src/ailego/math/norm2_matrix_fp16.cc |
Documents and gates FP16 NEON implementation away from MSVC ARM64. |
src/ailego/math/norm1_matrix.h |
Updates NEON/ARM64 guards for norm1 matrix specializations. |
src/ailego/math/norm1_matrix_fp32.cc |
Updates runtime dispatch to use AILEGO_HAVE_NEON. |
src/ailego/math/norm1_matrix_fp16.cc |
Documents and gates FP16 NEON implementation away from MSVC ARM64. |
src/ailego/math/mips_euclidean_distance_matrix_fp32_neon.cc |
Switches NEON compilation guard to AILEGO_HAVE_NEON. |
src/ailego/math/mips_euclidean_distance_matrix_fp32_dispatch.cc |
Switches NEON dispatch guard to AILEGO_HAVE_NEON. |
src/ailego/math/inner_product_matrix_fp32_neon.cc |
Switches NEON compilation guard to AILEGO_HAVE_NEON. |
src/ailego/math/inner_product_matrix_fp32_dispatch.cc |
Switches NEON dispatch guard to AILEGO_HAVE_NEON. |
src/ailego/math/euclidean_distance_matrix_fp32_neon.cc |
Switches NEON compilation guard to AILEGO_HAVE_NEON. |
src/ailego/math/euclidean_distance_matrix_fp32_dispatch.cc |
Switches NEON dispatch guard to AILEGO_HAVE_NEON. |
src/ailego/math/distance_matrix_fp32.i |
Updates NEON/AArch32 helper gating to use AILEGO_* macros. |
src/ailego/math/distance_matrix_accum_fp32.i |
Updates NEON/AArch32 vs AArch64 gating to use AILEGO_* macros. |
src/ailego/internal/cpu_features.cc |
Updates CPU feature detection guards to avoid x86 intrinsics on MSVC ARM64 (but needs an additional x86-only fix). |
src/ailego/CMakeLists.txt |
Removes MSVC-ARM early return so zvec_ailego target is always defined; keeps GCC/Clang-only -march handling. |
.github/workflows/05-windows-build.yml |
Expands Windows CI matrix to include an ARM64 job and parameterizes Python/MSVC arch per matrix entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cpu_features.cc selected the `__get_cpuid` / `__cpuid_count` constructor (and included <cpuid.h>) under `!defined(_MSC_VER) && !defined(__ARM_ARCH) && !defined(AILEGO_ARM64)`. That denylist is true on non-x86, non-ARM targets such as riscv64 and ppc64, where <cpuid.h> does not exist and the __get_cpuid intrinsics are unavailable -- a compile break (the linux-riscv CI job builds this file). Restore the original x86 allowlist `defined(__x86_64__) || defined(__i386__)` for both the include and the constructor. This still lands MSVC ARM64 on the empty-stub constructor (it defines neither __x86_64__ nor __i386__), which was the intent, while non-x86/non-ARM arches correctly fall through to the stub again. Reported by the Copilot PR reviewer.
There was a problem hiding this comment.
Good work! Could you also help update the newly merged code (e.g., fht*.cc) along with this?
Also, since the semantic AILEGO_HAVE_NEON / AILEGO_ARM64 macros are already introduced, would it be better to also wrap __ARM_NEON and aarch64 behind semantic macros? (e.g., AILEGO_ARM64_GNU_LIKE — feel free to suggest a better name if you have one.)
Extends the existing Windows job in 05-windows-build.yml to also cover `windows-11-arm` so the MSVC ARM64 build path is exercised on every PR, per request from @feihongxu0824 on alibaba#352. Changes: * Add a third row to the matrix with `platform: windows-11-arm`, `msvc_arch: arm64`, and `python_version: '3.11'` (Python 3.10 has no official Windows-on-ARM installer; 3.11 is the first). * Parameterize the existing `ilammy/msvc-dev-cmd@v1` step on `matrix.msvc_arch` instead of hard-coded `x64`, and the `actions/setup-python@v6` step on `matrix.python_version`. No changes to the x64 rows (still Python 3.10 + MSVC x64) and no changes to the build/test steps themselves — same `pip install -v .`, same C++ unittest run, same pytest, same examples. `fail-fast: false` was already set so an ARM64 regression will not hide x64 regressions and vice versa.
Addresses @feihongxu0824's review comment on alibaba#352. Previously the *_neon.cc files compiled into empty translation units on MSVC ARM64 because their guards (`__ARM_NEON`, `__aarch64__`) are GCC/Clang-only macros, leaving zvec to use the scalar fallback. That fallback hits ~1 ULP precision drift versus the NEON path, which surfaced as two HnswStreamerTest cosine failures and a `NormMatrix.Norm1_General` failure on the `windows-11-arm` CI runner. Changes: * Expand `defined(__ARM_NEON)` -> `(defined(__ARM_NEON) || defined(_M_ARM64))` at the 51 source sites that gate ARM NEON kernels (math, math_batch, utility, normalizer, platform headers, dispatch tables, version probe). * Expand `defined(__aarch64__)` -> `(defined(__aarch64__) || defined(_M_ARM64))` at the 26 sites that distinguish AArch64 from ARMv7 NEON — MSVC ARM64 is AArch64 but does not predefine `__aarch64__`. As a side effect the ARMv7-only polyfills (`vaddvq_f32`/`vaddvq_s32` shims in `distance_matrix_accum_fp32.i`, `distance_matrix_fp32.i`) are correctly skipped under MSVC ARM64, where those intrinsics are built in. * `src/include/zvec/ailego/internal/platform.h`: include `<arm_neon.h>` on the MSVC branch when `_M_ARM64` is defined (it was previously gated behind `!_MSC_VER`, so MSVC ARM64 saw no NEON types). * `src/ailego/CMakeLists.txt`: keep the existing `if(NOT MSVC)` wrapper around the GCC-only `-march=armv8-a` flag, and add an explicit `else()` branch with a comment explaining MSVC ARM64 does not need `-march` (NEON is the ARMv8 baseline on MSVC and the kernels are now picked up via the ALL_SRCS glob with the macro guards above). This supersedes the earlier `test(hnsw): skip two cosine self-match tests on MSVC ARM64` commit (which has been dropped from the branch). The NEON math kernels now use the same precision path as Linux/macOS ARM64, so those tests should pass natively on `windows-11-arm`.
The FHT rotator merged from main guards every `zvec::turbo::neon` function body with `__ARM_NEON && __aarch64__` and falls back to a no-op `(void)` stub. MSVC ARM64 defines neither macro, so `fht_rotate_neon` / `fht_unrotate_neon` returned without ever writing `out`, leaving the caller's buffer uninitialized rather than rotated. This was masked only by accident: `CpuFeatures::NEON()` also tested `__ARM_NEON`, so it reported false on MSVC ARM64 and turbo.cc fell back to the scalar rotator. That made `NEON()` disagree with `Intrinsics()` and `version.i`, which already report "Neon" from `AILEGO_HAVE_NEON` — so the obvious follow-up fix to `NEON()` would have activated the no-op path and silently corrupted results. Fix both halves together: - Guard the turbo NEON kernels with `AILEGO_HAVE_NEON && AILEGO_ARM64` so the real intrinsics compile on MSVC ARM64, and make every `#else` branch delegate to the scalar implementation instead of doing nothing. - Make `CpuFeatures::NEON()` use `AILEGO_HAVE_NEON`, consistent with `Intrinsics()` / `version.i`. - Build the sign-flip lane mask with `vld1q_u32`; MSVC models `uint32x4_t` as a union, so GCC-style brace initialization does not compile there. This surfaced once the kernels were actually built for MSVC ARM64. Also apply the same macros to the ailego FHT kernels (`fht_neon.cc`, `fht_dispatch.cc`), which were likewise left on the raw macros by the merge and therefore fell back to scalar on MSVC ARM64, and fix `mips_euclidean_distance_matrix_fp32_dispatch.cc`, where one call site still tested `__ARM_NEON` while its own forward declaration and sibling function already used `AILEGO_HAVE_NEON`. Verified on Windows ARM64 (MSVC): the NEON kernels and rotator now match the scalar reference bit-for-bit across power-of-2, non-power-of-2 and tail-remainder dimensions, and rotate/unrotate round-trips recover the input. Against the previous code the same check reports all-zero output, confirming the no-op. Changed files also compile clean for x64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback on alibaba#352. `__ARM_NEON && __aarch64__` appeared in ~10 guards meaning "AArch64 NEON under GCC/Clang", which is narrower than `AILEGO_HAVE_NEON && AILEGO_ARM64`: MSVC ARM64 has NEON but exposes neither `float16_t` nor the `v*_f16` intrinsics unless built for ARMv8.2 FP16. Give that condition a name, `AILEGO_ARM64_GNU_LIKE`, and document when to reach for it (FP16 kernels) versus `AILEGO_HAVE_NEON` (FP32 kernels, which MSVC supports). Substitutions are 1:1, so behaviour is unchanged on every target. The inner `#if defined(__ARM_NEON)` branch selectors inside FP16 blocks already gated on `(__F16C__ && __AVX__) || AILEGO_ARM64_GNU_LIKE` are equivalent within that block — `__F16C__` is x86-only and `__ARM_NEON` is ARM-only — so they move to the named macro too. Also refresh the trailing `#endif` comments left naming the old macros after the earlier conversion to `AILEGO_HAVE_NEON` / `AILEGO_ARM64`, which no longer matched the conditions directly above them. Left alone deliberately: the bare `__ARM_NEON` guards in the FP16 distance kernels (`*_fp16_neon.cc`, `*_fp16_dispatch.cc`). Those admit 32-bit ARMv7 as well as AArch64, so narrowing them to `AILEGO_ARM64_GNU_LIKE` would change behaviour on a platform this PR does not target and CI does not cover. They already evaluate false on MSVC ARM64, which is the correct result. Verified on Windows ARM64 (MSVC): `AILEGO_ARM64=1`, `AILEGO_HAVE_NEON=1`, `AILEGO_ARM64_GNU_LIKE=0`, so FP32 NEON kernels compile in while the FP16 ones stay excluded. Touched files compile clean for both ARM64 and x64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review @JalinWang — good catches, all addressed in 92c690f and 16aa7e4. Chasing the The
|
Review follow-ups on alibaba#352. Add two macros to platform.h and use them everywhere: - `AILEGO_ARM` for `__arm__ || AILEGO_ARM64`, the `ailego_yield()` guard. - `AILEGO_ARM64_NEON` for `AILEGO_HAVE_NEON && AILEGO_ARM64`, which the FP32 kernels repeated at 16 sites. Switch the `__fp16` paths in float_helper to `AILEGO_ARM64_GNU_LIKE`. The header and the source hold inverse guards and must agree, since they pick `Float16::value_`'s storage type, so float_helper.h now includes platform.h and names the same macro. Also reflow `fht_rotate_neon`'s signature, which drifted past clang-format when `out_dim` was un-commented, and drop the stray CRLF two lines of normalizer.cc picked up from an editor. Both broke the lint job. Substitutions are 1:1 on every supported target. Verified with clang-format 18.1.8, the version CI pins: all 784 checked files clean. On MSVC ARM64 `AILEGO_ARM64`, `AILEGO_ARM`, `AILEGO_HAVE_NEON` and `AILEGO_ARM64_NEON` are set and `AILEGO_ARM64_GNU_LIKE` is not; the turbo rotator's assembly listing carries NEON vector instructions, confirming the kernels compile in rather than falling back. Touched files build clean under /W4 for ARM64 and x64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Windows CI exposed two defects after CpuFeatures::NEON() began reporting true on MSVC ARM64. neon/pq_quantizer_int8/pq_distance.cc guarded on __ARM_NEON && __aarch64__, which MSVC never defines, and its #else branches returned without writing *out. Enabling NEON selected those stubs, so the ADC tests read uninitialised memory. The guards now use AILEGO_ARM64_NEON and the fallbacks delegate to the scalar kernels. pq_adc_int8_batch_distance_neon also scored every lane against chunk m's sub-table instead of chunks m..m+3, so results diverged from scalar once num_chunk reached 4. Each lane now loads its own table. A scalar-equivalence harness over 363 shape and value combinations reports no mismatches. fht_sse.cc, fht_avx2.cc and fht_avx512.cc test __SSE2__/__AVX2__/__AVX512F__ but did not include platform.h, which synthesises those macros for MSVC. fht_dispatch.cc did include it, so it called SSE entry points that compiled away to nothing (LNK2019). Including platform.h in the ISA files restores agreement and gives Windows x64 the SSE FHT path Linux x64 already uses. MSVC models NEON vector types as unions, so the GCC-style brace initialisers were replaced with vld1q_f32 over plain arrays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Merged NEON PQ kernels (
Also replaced the GCC-style vector brace initialisers, which MSVC rejects because it models NEON types as unions. Verified with clang-format 18.1.8 (clean), MSVC ARM64 and x64 compiles, and a link-and-run test of the FHT chain on x64. The |
…pport # Conflicts: # src/turbo/distance/neon/pq_quantizer_int8/pq_distance.cc
Summary
Enables
pip install .to produce a native ARM64 Python wheel on Windows ARM64 with MSVC + Visual Studio Build Tools 2022. Four small, targeted changes — three in zvec, one new patch file applied to the bundled Arrow 21.0 tree.The Python SDK build docs currently list Windows as x86_64-only with MSVC 2022+. This PR adds the ARM64 arm of that — everything else (Linux ARM64, macOS ARM64) is already working.
Changes
src/ailego/CMakeLists.txtarm/arm64branch ofAUTO_DETECT_ARCHhadif(MSVC) return() endif(), which bailed beforecc_library(zvec_ailego ...)was reached — downstream then failed withNo target "zvec_ailego". The-march=armv8-a/ NEON glob setup is still GCC/Clang-only, so wrap just that inif(NOT MSVC)and let the target definition proceed.src/ailego/internal/cpu_features.cc__cpuidex(x86/x64 intrinsic); GCC branch was gated only by!__ARM_ARCH. MSVC ARM64 matched neither and failed with__cpuidex: identifier not found. Scope both branches to x86/x64 explicitly so MSVC ARM64 falls through to the existing empty-stub ctor.thirdparty/arrow/CMakeLists.txtxsimd::make_sized_batch_tfor MSVC ARM64. Pass-DARROW_SIMD_LEVEL=NONE -DARROW_RUNTIME_SIMD_LEVEL=NONEonly whenCMAKE_SYSTEM_PROCESSORis ARM64 — x64 MSVC keeps its default SSE4.2 path untouched. Also wires in the new patch below.thirdparty/arrow/arrow.windows-arm64.patch(new)arrow/vendored/pcg/pcg_uint128.hpp) uses an x86-only endianness check and calls_umul128, which is not available on MSVC ARM64 — the equivalent intrinsic is__umulh. Add_M_ARM64/_M_ARM/__aarch64__/__arm__to the little-endian case, branch to__umulhon ARM, and guard#pragma intrinsic(_umul128)so it is not referenced on ARM. Applied via the existingapply_patch_oncemechanism, scoped to MSVC+ARM64.Non-goals
CMAKE_SYSTEM_PROCESSOR MATCHES "^(ARM64|arm64|aarch64)$"._M_ARM64using MSVC's<arm_neon.h>, but that's additive and out of scope here.IsProcessorFeaturePresentor compile-time__ARM_FEATURE_*macros.Test plan
Microsoft.VisualStudio.Component.VC.Tools.ARM64) —pip install .succeeds and produces a working wheelpython -c "import zvec; print(zvec)"imports cleanly on the resulting venv (zvec: OK (0.3.2.dev2))git apply --check thirdparty/arrow/arrow.windows-arm64.patchclean against the bundled Arrow 21.0 treeMSVC)Error trail for reviewers
For context on what each change fixes (in build order):
CMake Error at src/binding/c/CMakeLists.txt:109 (target_link_options): No target "zvec_ailego"→ fixed by ailego CMakeLists change.error C3861: '__cpuidex': identifier not found→ fixed by cpu_features.cc change.error C2039: 'make_sized_batch_t': is not a member of 'xsimd'(~20 errors inarrow_compute_core_staticandarrow_util_static) → fixed byARROW_SIMD_LEVEL=NONE.error C1189: #error: Unable to determine target endiannessinarrow/vendored/pcg/pcg_uint128.hpp→ fixed by endianness hunk of the new arrow patch.error C3861: '_umul128': identifier not foundin the same file → fixed by the__umulhhunk of the new arrow patch.After those five errors are resolved, the rest of the build (protobuf, rocksdb, arrow, zvec_core, zvec_db, zvec_ailego, zvec_turbo, roaring, Python binding) completes cleanly on MSVC ARM64.