BLAKE3-team / BLAKE3

the official Rust and C implementations of the BLAKE3 cryptographic hash function
Apache License 2.0
4.71k stars 315 forks source link

fix Windows ARM64 build and detect ARM64EC as ARM64 #389

Closed jblazquez closed 2 months ago

jblazquez commented 2 months ago

This PR solves two issues when building for Windows on ARM:

  1. A compilation error in blake3_dispatch.c due to a missing include.
  2. A misdetection of the ARM64EC ABI as AMD64.

The first issue manifests as follows. You can repro in an ARM64 Visual Studio Developer Command Prompt:

> cl.exe /nologo /diagnostics:caret /c blake3_dispatch.c

blake3_dispatch.c(106,16): error C2061: syntax error: identifier 'g_cpu_features'
    ATOMIC_INT g_cpu_features = UNDEFINED;
               ^
blake3_dispatch.c(106,16): error C2059: syntax error: ';'
blake3_dispatch.c(106,31): error C2513: ' ': no variable declared before '='
    ATOMIC_INT g_cpu_features = UNDEFINED;
                              ^
blake3_dispatch.c(115,31): error C2065: 'g_cpu_features': undeclared identifier
  enum cpu_feature features = ATOMIC_LOAD(g_cpu_features);

The second issue is more subtle. When building for the ARM64EC ABI, the compiler defines the _M_X64 macro and not the _M_ARM64 macro, to make incremental porting existing X64 Windows code to ARM easier.

This means that the BLAKE3 library was detecting ARM64EC as AMD64 and defining IS_X86 and IS_X86_64. This causes the library to compile in the SSE2 and SSE4.1 vector implementations of the algorithm instead of NEON, which amazingly does work even when building for Windows on ARM64, because Windows provides an emulated implementation of Intel SIMD instruction sets up to SSE4.2 in the softintrin.lib library. The end result is not ideal though, because it mixes native ARM64 code for the portable parts of the algorithm with emulated Intel SIMD intrinsics.

The second change is thus to make the CPU architecture detection in blake3_impl.h aware of the ARM64EC ABI so it can choose the NEON implementation.

jblazquez commented 2 months ago

Note that if #383 is merged after this change, it probably needs to change how it detects NEON so it doesn't just check for an __aarch64__ define and instead uses IS_AARCH64 from blake3_impl.h. Also, the inline ARM assembly won't be compatible with Windows on ARM and this other method would be needed.

oconnor663 commented 2 months ago

Thanks for the fix and for the clear explanation!

jblazquez commented 2 months ago

Thanks! Will add a note to the other PR I mentioned so it’s compatible with this change.

divinity76 commented 2 months ago

the inline ARM assembly won't be compatible with Windows on ARM

why not? modern MSVC doesn't support __asm__ ?

jblazquez commented 2 months ago

It doesn't, no. It used to support it for 32-bit Windows apps, but it never supported inline assembly for 64-bit Windows: https://learn.microsoft.com/en-us/cpp/assembler/inline/inline-assembler?view=msvc-170

Inline assembly is not supported on the ARM and x64 processors.

They provide intrinsics for many common uses of inline assembly. For all other use cases, you need to use separate assembly files.