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 compiling for Arm64 Windows with MSVC #483

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aaronfranke
Copy link

@aaronfranke aaronfranke commented Apr 10, 2024

This PR fixes 3 issues when compiling for Arm64 Windows with MSVC.

  • Fix detection of being a 64-bit platform. I replaced the defined(__X86_64__) || defined(__aarch64__) check with a platform-agnostic UINTPTR_MAX == UINT64_MAX that relies on stdint.h (please let me know if any of Embree's targets do not have stdint.h, my assumption is that this is fine to depend on, but I know that MSVC versions from before 2010 don't have it). MSVC does not define __aarch64__, but it does define _M_ARM64. Adding || defined(_M_ARM64) would also fix the problem, but it's best to make this platform-agnostic.
  • Always define unsigned 64-bit versions of bsf, bscf, and bsr. Without it, this function was not being defined on MSVC. Same as the above point, MSVC does not define __aarch64__, but it does define _M_ARM64. Adding || defined(_M_ARM64) would also fix the problem, but it's best to make this platform-agnostic. Using size_t with #if guards for 64-bit is the same as just using uint64_t. Note that functions may be automatically removed by the compiler if unused, so there is no reason to explicitly wrap it in #if guards.
  • Before #include <immintrin.h>, check if _M_ARM64 is defined (meaning Arm64 Windows MSVC) and if so define USE_SOFT_INTRINSICS. MSVC provides immintrin.h with software support on Arm64, but this has to be enabled explicitly. The MSVC-provided immintrin.h contains this code:
#if !defined(_M_IX86) && !defined(_M_X64) && !(defined(_M_ARM64) && defined(USE_SOFT_INTRINSICS))
#error This header is specific to X86, X64, ARM64, and ARM64EC targets
#endif

@aaronfranke
Copy link
Author

@freibold This PR is needed to fix Arm64 Windows builds with MSVC. Let me know if I can do anything to push this forward.

@freibold
Copy link
Contributor

freibold commented May 2, 2024

Hi @aaronfranke! Thanks for this PR. I had time to look at this end of last week. However, I had issues getting this to work using Visual Studio 2022 "ARM edition". It was compiling but the linker complained that it can't find "__mm_load_ps", "mm_store_ps", and other SSE intrinsic. I was hoping that adding the missing USE_SOFT_INTRINSICS in common/sys/thread.cpp would fix the issue but no luck so far. Did you add any additional CMake or CXX flags on your side?

On a more high-level I don't think the software intrinsic are the way to go. I was trying to enable ARM NEON via the SSE2NEON "library" like we do for Apple ARM but I hit a roadblock there, too. However, I think ARM NEON is the right for performance reasons. Anyhow, if you can help me out with getting your USE_SOFT_INTRINSICS solution to work, I'm happy to merge that in as a short-term Windows ARM solution.

@aaronfranke
Copy link
Author

@freibold Actually, that was where I got stuck too, it fails at the linker. godotengine/godot#91360 (comment)

At the time I submitted this PR (and at the time I wrote the ping to you) I had only tested compiling before the linker.

You are likely correct that the USE_SOFT_INTRINSICS stuff is completely wrong, but I believe the bsf, bscf, and bsr changes are correct, and the #if UINTPTR_MAX == UINT64_MAX change too. Hopefully this PR would be a good base for building a solution using Arm Neon. I'm completely lost when it comes to this, however. If you want I can remove the USE_SOFT_INTRINSICS from this PR so you can merge in the rest of the changes.

@aaronfranke aaronfranke marked this pull request as draft May 8, 2024 22:37
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

2 participants