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.

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

[Preprocessor] Fix __has_builtin for CPU ID functions #80058

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

nemanjai
Copy link
Member

My recent commit (67c1c1d) made the CPU ID builtins target-independent so they can be used on PPC as well. However, that had the unintended consequence of changing the behaviour of __has_builtin in that it reports these as supported at the pre-processor level. This makes it impossible to guard the use of these with this feature test macro which is clearly not ideal.
This patch restores the behaviour of __has_builtin for __builtin_cpu_is, __builtin_cpu_init,
__builtin_cpu_supports. Now the preprocessor queries the target to determine whether the target supports the builtin.

My recent commit (67c1c1d) made the CPU ID builtins
target-independent so they can be used on PPC as well.
However, that had the unintended consequence of changing
the behaviour of __has_builtin in that it reports these
as supported at the pre-processor level. This makes it
impossible to guard the use of these with this feature
test macro which is clearly not ideal.
This patch restores the behaviour of __has_builtin
for __builtin_cpu_is, __builtin_cpu_init,
__builtin_cpu_supports. Now the preprocessor queries
the target to determine whether the target supports
the builtin.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 30, 2024
@nemanjai nemanjai self-assigned this Jan 30, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 30, 2024

@llvm/pr-subscribers-clang

Author: Nemanja Ivanovic (nemanjai)

Changes

My recent commit (67c1c1d) made the CPU ID builtins target-independent so they can be used on PPC as well. However, that had the unintended consequence of changing the behaviour of __has_builtin in that it reports these as supported at the pre-processor level. This makes it impossible to guard the use of these with this feature test macro which is clearly not ideal.
This patch restores the behaviour of __has_builtin for __builtin_cpu_is, __builtin_cpu_init,
__builtin_cpu_supports. Now the preprocessor queries the target to determine whether the target supports the builtin.


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

2 Files Affected:

  • (modified) clang/lib/Lex/PPMacroExpansion.cpp (+6)
  • (added) clang/test/Preprocessor/has_builtin_cpuid.c (+20)
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index ad02f31209b0b..3017461dc66e8 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1672,6 +1672,12 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
           return false;
         else if (II->getBuiltinID() != 0) {
           switch (II->getBuiltinID()) {
+          case Builtin::BI__builtin_cpu_is:
+            return getTargetInfo().supportsCpuIs();
+          case Builtin::BI__builtin_cpu_init:
+            return getTargetInfo().supportsCpuInit();
+          case Builtin::BI__builtin_cpu_supports:
+            return getTargetInfo().supportsCpuSupports();
           case Builtin::BI__builtin_operator_new:
           case Builtin::BI__builtin_operator_delete:
             // denotes date of behavior change to support calling arbitrary
diff --git a/clang/test/Preprocessor/has_builtin_cpuid.c b/clang/test/Preprocessor/has_builtin_cpuid.c
new file mode 100644
index 0000000000000..8de6331e62d6e
--- /dev/null
+++ b/clang/test/Preprocessor/has_builtin_cpuid.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -triple arm64-- -DARM -verify %s
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-- -DX86 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -triple powerpc64-unknown-linux-gnu -DPPC \
+// RUN:   -verify %s
+// expected-no-diagnostics
+#if __has_builtin(__builtin_cpu_is)
+# ifdef ARM
+#   error "ARM shouldn't have __builtin_cpu_is"
+# endif
+#endif
+#if __has_builtin(__builtin_cpu_init)
+# if defined(ARM) || defined(PPC)
+#   error "ARM/PPC shouldn't have __builtin_cpu_init"
+# endif
+#endif
+#if __has_builtin(__builtin_cpu_supports)
+# ifdef ARM
+#   error "ARM shouldn't have __builtin_cpu_supports"
+# endif
+#endif

Copy link
Contributor

@lei137 lei137 left a comment

Choose a reason for hiding this comment

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

LGTM
Thx for the quick fix.

@alexfh
Copy link
Contributor

alexfh commented Jan 31, 2024

LGTM. Thank you for the quick fix!

@eaeltsin
Copy link
Contributor

eaeltsin commented Feb 2, 2024

Can we please have this fixed and submitted soon? Thanks!

@nemanjai nemanjai merged commit a52e9ec into llvm:main Feb 2, 2024
5 of 6 checks passed
@nemanjai nemanjai deleted the nemanja/fix_has_builtin branch February 2, 2024 09:49
agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
My recent commit (67c1c1d) made the CPU ID builtins target-independent
so they can be used on PPC as well. However, that had the unintended
consequence of changing the behaviour of __has_builtin in that it
reports these as supported at the pre-processor level. This makes it
impossible to guard the use of these with this feature test macro which
is clearly not ideal.
This patch restores the behaviour of __has_builtin for __builtin_cpu_is,
__builtin_cpu_init,
__builtin_cpu_supports. Now the preprocessor queries the target to
determine whether the target supports the builtin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants