-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Clang] Fix __cpuidex conflict with other offloading targets #157741
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
Signed-off-by: Sarnie, Nick <[email protected]>
// builtin. Given __has_builtin does not detect builtins on aux triples, we need | ||
// to explicitly check for some offloading cases. | ||
#ifndef __NVPTX__ | ||
#if !defined(__NVPTX__) && !defined(__AMDGPU__) && !defined(__SPIRV__) |
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.
Another option is to add a TARGET_OFFLOADING
macro or something and check that, let me know what you prefer.
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-clang Author: Nick Sarnie (sarnex) ChangesIt seems that for whatever reason we must:
In Previously a workaround was added for NVPTX in #152556, extend it for the other offloading targets. Full diff: https://github.com/llvm/llvm-project/pull/157741.diff 2 Files Affected:
diff --git a/clang/lib/Headers/cpuid.h b/clang/lib/Headers/cpuid.h
index ce8c79e77dc18..45700c635831d 100644
--- a/clang/lib/Headers/cpuid.h
+++ b/clang/lib/Headers/cpuid.h
@@ -348,7 +348,7 @@ static __inline int __get_cpuid_count (unsigned int __leaf,
// In some cases, offloading will set the host as the aux triple and define the
// builtin. Given __has_builtin does not detect builtins on aux triples, we need
// to explicitly check for some offloading cases.
-#ifndef __NVPTX__
+#if !defined(__NVPTX__) && !defined(__AMDGPU__) && !defined(__SPIRV__)
static __inline void __cpuidex(int __cpu_info[4], int __leaf, int __subleaf) {
__cpuid_count(__leaf, __subleaf, __cpu_info[0], __cpu_info[1], __cpu_info[2],
__cpu_info[3]);
diff --git a/clang/test/Headers/__cpuidex_conflict.c b/clang/test/Headers/__cpuidex_conflict.c
index 67f2a0cf908e5..a928aa895c44d 100644
--- a/clang/test/Headers/__cpuidex_conflict.c
+++ b/clang/test/Headers/__cpuidex_conflict.c
@@ -6,6 +6,9 @@
// Ensure that we do not run into conflicts when offloading.
// RUN: %clang_cc1 %s -DIS_STATIC=static -ffreestanding -fopenmp -fopenmp-is-target-device -aux-triple x86_64-unknown-linux-gnu
// RUN: %clang_cc1 -DIS_STATIC="" -triple nvptx64-nvidia-cuda -aux-triple x86_64-unknown-linux-gnu -aux-target-cpu x86-64 -fcuda-is-device -x cuda %s -o -
+// RUN: %clang_cc1 -DIS_STATIC="" -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-linux-gnu -aux-target-cpu x86-64 -fcuda-is-device -x cuda %s -o -
+// RUN: %clang_cc1 -DIS_STATIC="" -triple spirv64 -aux-triple x86_64-unknown-linux-gnu -aux-target-cpu x86-64 -fcuda-is-device -x cuda %s -o -
+// RUN: %clang_cc1 -DIS_STATIC="" -triple spirv64 -aux-triple x86_64-unknown-linux-gnu -aux-target-cpu x86-64 -fsycl-is-device %s -o -
typedef __SIZE_TYPE__ size_t;
|
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.
Seems reasonable enough to me. I might try a more principled fix in clang/lib/Basic/Builtins.cpp
again at some point, but that needs some more thinking and this works well for now.
Thanks Aiden, yeah I think the root issue here is just how we handle builtins for offloading targets, what we are doing seems very unexpected to me, if I make it do what I expect I see <20 lit fails, so maybe it's possible we could rework it to make sense, but yeah requires further investigation. |
It seems that for whatever reason we must:
and
In
cpuid.h
we try to define__cpuidex
if it is not defined. Given the above, the function will both be defined as a builtin in the compiler and we can't rely on theX86
macros to be undefined in the case the aux-triple isX86
.Previously a workaround was added for NVPTX in #152556, extend it for the other offloading targets.