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.

Sign up for GitHub

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

Jump to bottom

Fix some ARM/clang-cl feature detection issues #623

Merged
merged 2 commits into from Dec 1, 2021
Merged

Fix some ARM/clang-cl feature detection issues #623

merged 2 commits into from Dec 1, 2021

Conversation

easyaspi314
Copy link
Contributor

@easyaspi314 easyaspi314 commented Nov 30, 2021 •
edited

  • Fix Windows ARM build bustage with xxHash 0.8.1 #620 where clang-cl did not include <arm_neon.h>
  • Use __uint128_t for clang-cl
  • Use NEON when _M_ARM is defined, not only _M_ARM_ARMV7VE
  • Use ARM64 codepaths for the new ARM64EC ABI in VS2022 Preview (which emulates the x64 ABI and is detected as so)
    • TODO: don't dispatch
  • Update CLI detection to detect Windows ARM better (although you have to cheat to actually run the program on Windows RT lol, only wine users will see it)

This should probably be merged into a hotfix for v0.8.1.

@easyaspi314
Fix some feature detection for arm/windows
85769a8 
 - Fix some clang-cl misfires
 - Include `<arm_neon.h>` in clang-cl, fixes Cyan4973#620
 - `_M_ARM` implies NEON, no need to check for ARMv7VE
 - Detect the `_M_ARM64EC` target
  - New ABI for Windows ARM64 which is linkable with x64 code
  - Otherwise indistinguishable from x64 and therefore would attempt the SSE2 path
 - Remove a redundant #include
@easyaspi314
Fix CLI detection for Windows ARM and ARM64EC
2e7af13 
Not that you would get an ARMv7 binary to run in cmd but it fixes
it nonetheless.
Cyan4973
Cyan4973 approved these changes Dec 1, 2021
View reviewed changes
Copy link
Owner

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

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

The fix itself is good.

I'm just wondering if we should pair it with some kind of extended test coverage, triggering the problem that's been fixed in this PR.

@t-mat
Copy link
Contributor

t-mat commented Dec 1, 2021

I'm just wondering if we should pair it with some kind of extended test coverage, triggering the problem that's been fixed in this PR.

#637 may mitigate your concern. But since the root cause of #620 is related to specific version of MSVC (and its header), #637 doesn't reproduce the issue.

As for MSVC 2019 and later, we can download specific version of it (as a buildtools). Though it takes some amount of time for downloading.
But MSVC 2017 provides a few (incomplete) versions of their releases.
Perhaps they provides complete list in the Azure archive. But I don't know anything about it.

@Cyan4973 Cyan4973 merged commit b3137c4 into Cyan4973:dev Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows ARM build bustage with xxHash 0.8.1
3 participants
@easyaspi314 @t-mat @Cyan4973