-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix build when targeting Arm64EC using Clang #2012
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
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@@ -79,7 +79,7 @@ inline BENCHMARK_ALWAYS_INLINE int64_t Now() { | |||
int64_t ret; | |||
__asm__ volatile("rdtsc" : "=A"(ret)); | |||
return ret; | |||
#elif defined(__x86_64__) || defined(__amd64__) | |||
#elif (defined(__x86_64__) || defined(__amd64__)) && !defined(__arm64ec__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is __x86_64__
or __amd64__
really defined when __arm64ec__
is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should think of Arm64EC as "x64 code running on an Arm64 machine with a weird instruction set". So, by default, it needs to use the x64 layout for objects to be compatible with any real x64 code that is linked into the same binary or called across binaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but ARM is ARM, it is not Intel arch, which x86_64 and AMD64 are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a code comment to explicitly state this, err, very interesting design decision...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@dpaoliello thank you! |
These fix building benchmark targeting Arm64EC Windows when using Clang (i.e., with Clang's preprocessor defines rather than the MSVC-compatibility defines).
Copied from llvm/llvm-project#150068