This is an archive of the discontinued LLVM Phabricator instance.

[Support, ARM64] Define getHostCPUFeatures for Windows on ARM64 platform
ClosedPublic

Authored by kaadam on Sep 27 2019, 6:53 AM.

Details

Summary

This change implements getHostCPUFeatures function to enable this functionality on Windows on ARM64. Unittest isn't included for this patch, since I'm not able to see how to reasonably write one, and I couldn't find any existing testcases related to it.

Diff Detail

Event Timeline

kaadam created this revision.Sep 27 2019, 6:53 AM

Hi Michael,

This is my first contribution into llvm, and I'm not sure if I should add someone else to reviewers.

Could you look at this? Or could you suggest anyone?

Many thanks,
Adam

For what it's worth I think that this looks right. I can't find a convenient link to the Microsoft documentation that defines the PF_ARM_* defines. However looking at https://groups.google.com/a/chromium.org/forum/#!topic/crashpad-dev/yZ2RV3mIkPY they look correct and complete. I think Martin Storsjo has done a lot of work on Windows support in LLVM, I'd defer to him for approval.

Another set of people that might be able to help are those that do the Win32 support in Clang for X86. Reid Kleckner comes to mind as one of the main contributors.

mstorsjo added inline comments.Oct 1 2019, 8:06 AM
lib/Support/Host.cpp
1515 ↗(On Diff #222159)

This should be defined(_WIN32) && (defined(__aarch64__) || defined(_M_ARM64)), otherwise it won't be used by MSVC.

Also, those PF_* constants are missing in mingw-w64; I'll have to add them there in order not to break builds with such headers.

TomTan added inline comments.Oct 1 2019, 5:45 PM
lib/Support/Host.cpp
1515 ↗(On Diff #222159)

As Win32 API is called here, a single check on _M_ARM64 would be enough for clang and MSVC, but I am not sure for mingw-w64.

mstorsjo added inline comments.Oct 1 2019, 10:58 PM
lib/Support/Host.cpp
1515 ↗(On Diff #222159)

In general terms, the _M_* defines aren't predefied by clang in MinGW mode, or GCC. The MinGW headers do define _M_* defines though (but exactly which header defines them differs between mingw.org and mingw-w64 headers). It's good practice to always check for both arch defines in sync (except for strictly msvc/clang-cl cases, i.e. ifdef _MSC_VER), for consistency.

kaadam updated this revision to Diff 222762.Oct 1 2019, 11:50 PM
kaadam marked an inline comment as done.Oct 1 2019, 11:52 PM

Martin, Tom thanks for your comment.

Martin, I've updated the diff based on your review.

mstorsjo accepted this revision.Oct 2 2019, 3:00 AM

LGTM.

Do you have commit access, or do you need someone else to commit it for you?

This revision is now accepted and ready to land.Oct 2 2019, 3:00 AM
kaadam added a comment.Oct 2 2019, 3:35 AM

Thanks Martin. I don't have permission to do that. May I ask you to commit it?

This revision was automatically updated to reflect the committed changes.