Skip to content

Conversation

boomanaiden154
Copy link
Contributor

It does not look like __cpuidex builtins are being incorrectly included in compilations for offload targets anymore, so change up the test to assume that we are defining __cpuidex as static in cpuid.h now that #126324 updates the behavior of __has_builtin on offload compilations.

This ensures we are still testing that we are avoiding the conflicts around offloading that were first pointed out in
https://reviews.llvm.org/D150646.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jul 29, 2025
It does not look like __cpuidex builtins are being incorrectly included
in compilations for offload targets anymore, so change up the test to
assume that we are defining __cpuidex as static in cpuid.h now that
\llvm#126324 updates the behavior of __has_builtin on offload compilations.

This ensures we are still testing that we are avoiding the conflicts
around offloading that were first pointed out in
https://reviews.llvm.org/D150646.
@boomanaiden154 boomanaiden154 force-pushed the cpuidex-conflict-test-remove-target-stuff branch from 372daf3 to f540525 Compare July 29, 2025 20:14
@llvmbot
Copy link
Member

llvmbot commented Jul 29, 2025

@llvm/pr-subscribers-clang

Author: Aiden Grossman (boomanaiden154)

Changes

It does not look like __cpuidex builtins are being incorrectly included in compilations for offload targets anymore, so change up the test to assume that we are defining __cpuidex as static in cpuid.h now that #126324 updates the behavior of __has_builtin on offload compilations.

This ensures we are still testing that we are avoiding the conflicts around offloading that were first pointed out in
https://reviews.llvm.org/D150646.


Full diff: https://github.com/llvm/llvm-project/pull/151220.diff

1 Files Affected:

  • (modified) clang/test/Headers/__cpuidex_conflict.c (+6-9)
diff --git a/clang/test/Headers/__cpuidex_conflict.c b/clang/test/Headers/__cpuidex_conflict.c
index 49795c447f6e0..74f45327de2bb 100644
--- a/clang/test/Headers/__cpuidex_conflict.c
+++ b/clang/test/Headers/__cpuidex_conflict.c
@@ -1,19 +1,17 @@
 // Make sure that __cpuidex in cpuid.h doesn't conflict with the MS
 // extensions built in by ensuring compilation succeeds:
-// RUN: %clang_cc1 %s -ffreestanding -fms-extensions -fms-compatibility \
-// RUN:  -fms-compatibility-version=19.00 -triple x86_64-pc-windows-msvc -emit-llvm -o -
-// %clang_cc1 %s -ffreestanding -triple x86_64-w64-windows-gnu -fms-extensions -emit-llvm -o -
-//
-// FIXME: See https://github.com/llvm/llvm-project/pull/121839 and
-// FIXME: https://github.com/llvm/llvm-project/pull/126324
-// RUN: not %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device -aux-triple x86_64-unknown-linux-gnu
+// RUN: %clang_cc1 %s -DIS_STATIC="" -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=19.00 -triple x86_64-pc-windows-msvc -emit-llvm -o -
+// RUN: %clang_cc1 %s -DIS_STATIC="" -ffreestanding -triple x86_64-w64-windows-gnu -fms-extensions -emit-llvm -o -
+
+// 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
 
 typedef __SIZE_TYPE__ size_t;
 
 // We declare __cpuidex here as where the buitlin should be exposed (MSVC), the
 // declaration is in <intrin.h>, but <intrin.h> is not available from all the
 // targets that are being tested here.
-void __cpuidex (int[4], int, int);
+IS_STATIC void __cpuidex (int[4], int, int);
 
 #include <cpuid.h>
 
@@ -22,4 +20,3 @@ int cpuid_info[4];
 void test_cpuidex(unsigned level, unsigned count) {
   __cpuidex(cpuid_info, level, count);
 }
-

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

This LGTM but I'd like @sarnex to give the final approval.

Copy link
Member

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

LGTM assuming the current (and maybe previous?) goal of the offload part of this test is to lock down that __cpuidex isn't declared as a builtin when offloading causing a conflict with intrin.h (well not actually using intrin.h here for the commented reason) because the definition there is static and the builtin is not.

My understanding is that the offload part of this test does not depend on nor is it testing anything related to what the __has_builtin(__cpuidex) in cpuid.h returns when offloading. The test should fail if the builtin is declared no matter what happens in cpuid.h.

If that's correct feel free to merge. Thanks so much!

@boomanaiden154
Copy link
Contributor Author

Yep, that's exactly the reason.

@boomanaiden154 boomanaiden154 merged commit 3b5aff5 into llvm:main Jul 30, 2025
9 checks passed
@boomanaiden154 boomanaiden154 deleted the cpuidex-conflict-test-remove-target-stuff branch July 30, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants