Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Windows ARM64 build and detect ARM64EC as ARM64 #389

Merged
merged 1 commit into from Apr 7, 2024

Conversation

jblazquez
Copy link
Contributor

@jblazquez jblazquez commented Apr 7, 2024

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.

@@ -4,9 +4,12 @@

#include "blake3_impl.h"

#if defined(IS_X86)
#if defined(_MSC_VER)
#include <Windows.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this Windows.h include out of the IS_X86 block below.

@jblazquez
Copy link
Contributor Author

jblazquez commented Apr 7, 2024

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 oconnor663 merged commit 0816bad into BLAKE3-team:master Apr 7, 2024
51 checks passed
@oconnor663
Copy link
Member

Thanks for the fix and for the clear explanation!

@jblazquez
Copy link
Contributor Author

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

@divinity76
Copy link
Contributor

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

why not? modern MSVC doesn't support __asm__ ?

@jblazquez
Copy link
Contributor Author

jblazquez commented Apr 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants