[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-27 Thread Nick Sarnie via cfe-commits 

sarnex wrote:

Yeah seems there's no easy fix here.

Revert PR here https://github.com/llvm/llvm-project/pull/124626

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-27 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

> This breaks CUDA compilation on ARM, because `__has_builtin()` now returns 
> false for the host-side builtins and that causes some clang headers on ARM to 
> try defining their own replacement for the builtin they consider to be 
> missing, but which is actually still there: https://godbolt.org/z/v9qffds1Y
> 
> https://github.com/llvm/llvm-project/blob/285009f202ca8bfcc6b607eba0e919867559e725/clang/lib/Headers/arm_acle.h#L44-L48

Right, the curse of merge compilation strikes again. Feels like damned if you 
do damned if you don't here. It's either broken inside of the device or outside 
of the device. It feels rather unfortunate to not have a mechanism to determine 
if we can actually *generate code* for a builtin. Perhaps we could make some 
offloading specific hack to work around that, otherwise it's tough to make 
generic headers that actually work.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-27 Thread Artem Belevich via cfe-commits 

Artem-B wrote:

I think the conceptual problem here is that `__has_builtin()` conflates 
"builtin exists" with "builtin is usable on the target". For C++ they are the 
same. For CUDA they are not. Builting in aux-triple do exist (as in compiler 
does see them when it constructs AST), but we can't generate code for them.

Considering that `__has_builtin` is a preprocessor entity and applies to all 
source code, it can never mean `is it usable` and can only indicate that the 
builtin is visible to the compiler. It can never tall whether the builtin is 
usable because for CUDA it would require the caller context, which preprocessor 
does not have.

I think this patch should be reverted.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-27 Thread Artem Belevich via cfe-commits 

Artem-B wrote:

This breaks CUDA compilation on ARM, because `__has_builtin()` now returns 
false for the host-side builtins and that causes some clang headers on ARM to 
try defining their own replacement for the builtin they consider to be missing, 
but which is actually still there: https://godbolt.org/z/v9qffds1Y

https://github.com/llvm/llvm-project/blob/285009f202ca8bfcc6b607eba0e919867559e725/clang/lib/Headers/arm_acle.h#L44-L48


https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex closed 
https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 


@@ -3,7 +3,10 @@
 // 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 -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+//
+// TODO(boomanaiden154)

sarnex wrote:

fixed in 
https://github.com/llvm/llvm-project/commit/0ee8d8cd4b3d61fc6cf6e740a54dd5a23c921048!

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex deleted 
https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 


@@ -3,7 +3,10 @@
 // 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 -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+//
+// TODO(boomanaiden154)

sarnex wrote:

fixed in 
https://github.com/llvm/llvm-project/pull/121839/commits/0ee8d8cd4b3d61fc6cf6e740a54dd5a23c921048

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex updated 
https://github.com/llvm/llvm-project/pull/121839

>From 997373350540448d91f9884b98cbdc0df058a7a3 Mon Sep 17 00:00:00 2001
From: "Sarnie, Nick" 
Date: Mon, 6 Jan 2025 11:48:07 -0800
Subject: [PATCH 1/5] [Clang] __has_builtin should return false for aux triple
 builtins

Signed-off-by: Sarnie, Nick 
---
 clang/include/clang/Basic/Builtins.h |  5 +
 clang/lib/Basic/Builtins.cpp | 19 +++
 clang/lib/Lex/PPMacroExpansion.cpp   | 20 +---
 clang/test/Preprocessor/builtin_aux_info.cpp | 12 
 4 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Preprocessor/builtin_aux_info.cpp

diff --git a/clang/include/clang/Basic/Builtins.h 
b/clang/include/clang/Basic/Builtins.h
index 63559d977ce6b6..0939f95b0922c1 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -74,6 +74,7 @@ struct Info {
   const char *Features;
   HeaderDesc Header;
   LanguageID Langs;
+  bool operator==(const Info &Other) const;
 };
 
 /// Holds information about both target-independent and
@@ -268,6 +269,10 @@ class Context {
   /// for AuxTarget).
   unsigned getAuxBuiltinID(unsigned ID) const { return ID - TSRecords.size(); }
 
+  // Return true if the AuxBuiltin ID represents a target-specific builtin that
+  // is always unsupported on the default target.
+  bool isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(unsigned ID) const;
+
   /// Returns true if this is a libc/libm function without the '__builtin_'
   /// prefix.
   static bool isBuiltinFunc(llvm::StringRef Name);
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 588183788de322..c6e54b89e848cc 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -41,6 +41,14 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 #include "clang/Basic/Builtins.inc"
 };
 
+bool Builtin::Info::operator==(const Builtin::Info &Other) const {
+  auto StrCompare = [](StringRef A, StringRef B) { return A == B; };
+  return Name == Other.Name && StrCompare(Type, Other.Type) &&
+ StrCompare(Attributes, Other.Attributes) &&
+ StrCompare(Features, Other.Features) && Header.ID == Other.Header.ID 
&&
+ Langs == Other.Langs;
+}
+
 const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
   if (ID < Builtin::FirstTSBuiltin)
 return BuiltinInfo[ID];
@@ -183,6 +191,17 @@ unsigned Builtin::Context::getRequiredVectorWidth(unsigned 
ID) const {
   return Width;
 }
 
+bool Builtin::Context::isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+unsigned ID) const {
+  assert(isAuxTargetBuiltinID(ID) && "Expected aux target builtin ID");
+  const auto &Record = getRecord(ID);
+  for (const auto &MainTargetBuiltin : TSRecords)
+if (Record == MainTargetBuiltin)
+  return false;
+
+  return true;
+}
+
 bool Builtin::Context::isLike(unsigned ID, unsigned &FormatIdx,
   bool &HasVAListArg, const char *Fmt) const {
   assert(Fmt && "Not passed a format string");
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index 347c13da0ad215..13d9a0094a5827 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1804,8 +1804,9 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
diag::err_feature_check_malformed);
 if (!II)
   return false;
-else if (II->getBuiltinID() != 0) {
-  switch (II->getBuiltinID()) {
+auto BuiltinID = II->getBuiltinID();
+if (BuiltinID != 0) {
+  switch (BuiltinID) {
   case Builtin::BI__builtin_cpu_is:
 return getTargetInfo().supportsCpuIs();
   case Builtin::BI__builtin_cpu_init:
@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+BuiltinID))
+  return false;
 return Builtin::evaluateRequiredTargetFeatures(
-

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 


@@ -3,7 +3,10 @@
 // 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 -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+//
+// TODO(boomanaiden154)

sarnex wrote:

sure, will do!

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Erich Keane via cfe-commits 


@@ -3,7 +3,10 @@
 // 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 -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+//
+// TODO(boomanaiden154)

erichkeane wrote:

Rather than a `TODO` and the name, a FIXME + the issue should be sufficient.  
Names like that in source are one of those things that age poorly :) 

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Nick Sarnie via cfe-commits 


@@ -3,7 +3,10 @@
 // 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 -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+//
+// TODO(boomanaiden154)

sarnex wrote:

Thanks for the review! 

@boomanaiden154 specifically suggested this 
[here](https://github.com/llvm/llvm-project/pull/121839#discussion_r1925920983),
 but I'm happy to change it to FIXME if you prefer.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Erich Keane via cfe-commits 


@@ -3,7 +3,10 @@
 // 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 -
-// RUN: %clang_cc1 %s -ffreestanding -fopenmp -fopenmp-is-target-device 
-aux-triple x86_64-unknown-linux-gnu
+//
+// TODO(boomanaiden154)

erichkeane wrote:

TODO with a name in it?  Typically we just do a 'fixme', particularly awkward 
to call out other folks in the TODO.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Erich Keane via cfe-commits 

https://github.com/erichkeane approved this pull request.

1 nit, else lgtm.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-23 Thread Erich Keane via cfe-commits 

https://github.com/erichkeane edited 
https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Aiden Grossman via cfe-commits 

https://github.com/boomanaiden154 approved this pull request.

Changes to `__cpuidex_conflict.c` LGTM. I will put that on my TODO list.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Nick Sarnie via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

sarnex wrote:

@boomanaiden154 If we break the aux triple use part of the test, do you expect 
it to affect actual users or could we just XFAIL it and proceed with this 
change (assuming everyone agrees the proposed behavior is correct) and fix it 
later?

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Aaron Ballman via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

AaronBallman wrote:

It may be that gating it on something other than the triple is undesirable; I'm 
not certain. The point is more that builtin authors specify what language modes 
the builtin should be exposed in and exposing the builtins in other modes than 
that is really hard to reason about.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Aiden Grossman via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

boomanaiden154 wrote:

I remember it being difficult/undesirable to gate this on the presence of 
`-fms-extensions`. I had a patch up that did that, and I remember @rnk 
commenting that wasn't the right approach. I need to go dig the stuff up and my 
bandwidth is a bit limited right now.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Aaron Ballman via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

AaronBallman wrote:

I think this behavior (https://godbolt.org/z/oEx4Earqj) is surprising in that 
it's unclear to me why `__cpuidex` is exposed as a builtin given that it's only 
a builtin on all MS languages 
(https://github.com/llvm/llvm-project/blob/f63e8ed16ef1fd2deb80cd88b5ca9d5b631b1c36/clang/include/clang/Basic/BuiltinsX86.td#L4494)
 and the aux triple that is given is not an MS language (it's 
x86_64-unknown-linux-gnu). It would make sense to me if the user passed 
`-fms-extensions` (or `-fms-compatibility` I suppose). The fact that 
`__has_builtin` returns `true` and we get the error *does* make sense (it would 
be weird for it to return false and still get the error or for it to return 
true and not get the error), but I think that `__has_builtin` should return 
`false` in the case where MS language extensions aren't enabled.

> The change in question uses the ifdef to try to prevent double definitions 
> here,

Yup!

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Nick Sarnie via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

sarnex wrote:

Sorry for the late response.

My interpretation of Aaron's comment seems like he expects the behavior we see 
today (aux triple builtins enabled on offloading targets), thus making the 
OpenMP offloading part of the `cpuid` LIT test correct.

@AaronBallman Do you mind clarifying what you think should  happen here? Sorry, 
your comment was from a while back.


https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Nick Sarnie via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

sarnex wrote:

The change in question uses the `ifdef` to try to prevent double definitions 
[here](https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/cpuid.h#L345),
 so if we change the return value then it would break that code (which was 
caught by the LIT fail), and reading the above PR it seems it was hard to find 
a check that worked, so I'm not sure what's best.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-22 Thread Joseph Huber via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

jhuber6 wrote:

We want them enabled, but the ifdef should likely return false here, otherwise 
any routines that use `__has_builtin` don't work when put on the GPU and that 
would be really unfortunate.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-13 Thread Nick Sarnie via cfe-commits 

sarnex wrote:

> I believe the conclusion was that we should emit the builtin functions, but 
> `__has_builtin` should return false for builtins only present in the aux 
> triple. For the one test that complicates this behavior, I don't think the 
> use-case is completely well-formed but was hoping @boomanaiden154 would chime 
> in on what the expected behavior is. I would prefer if this is just a simply 
> check for "not in aux builtin".

Thanks, hopefully we can figure out the intended behavior.



https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-13 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

> Sorry, I kind of got lost in the above discussion. Are additional changes 
> required in Clang to parse builtins unsupported on the current target to make 
> sure we don't break anything or is only changing the result of 
> `__has_builtin` acceptable?

I believe the conclusion was that we should emit the builtin functions, but 
`__has_builtin` should return false for builtins only present in the aux 
triple. For the one test that complicates this behavior, I don't think the 
use-case is completely well-formed but was hoping @boomanaiden154 would chime 
in on what the expected behavior is. I would prefer if this is just a simply 
check for "not in aux builtin".

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-13 Thread Nick Sarnie via cfe-commits 

sarnex wrote:

Sorry, I kind of got lost in the above discussion. Are additional changes 
required in Clang to parse builtins unsupported on the current target to make 
sure we don't break anything or is only changing the result of `__has_builtin` 
acceptable?

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Nick Sarnie via cfe-commits 


@@ -0,0 +1,12 @@
+// REQUIRES: spirv-registered-target
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -fopenmp -triple=spirv64 -fopenmp-is-target-device \
+// RUN: -aux-triple x86_64-linux-unknown -E %s | FileCheck 
-implicit-check-not=BAD %s

sarnex wrote:

hopefully addressed in 
https://github.com/llvm/llvm-project/pull/121839/commits/3ce51118035f1f84ad02898858c95021ff6420d9,
 thx

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Nick Sarnie via cfe-commits 


@@ -0,0 +1,12 @@
+// REQUIRES: spirv-registered-target
+// REQUIRES: x86-registered-target
+

sarnex wrote:

fixed in 
https://github.com/llvm/llvm-project/pull/121839/commits/3ce51118035f1f84ad02898858c95021ff6420d9,
 thx

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex updated 
https://github.com/llvm/llvm-project/pull/121839

>From 997373350540448d91f9884b98cbdc0df058a7a3 Mon Sep 17 00:00:00 2001
From: "Sarnie, Nick" 
Date: Mon, 6 Jan 2025 11:48:07 -0800
Subject: [PATCH 1/3] [Clang] __has_builtin should return false for aux triple
 builtins

Signed-off-by: Sarnie, Nick 
---
 clang/include/clang/Basic/Builtins.h |  5 +
 clang/lib/Basic/Builtins.cpp | 19 +++
 clang/lib/Lex/PPMacroExpansion.cpp   | 20 +---
 clang/test/Preprocessor/builtin_aux_info.cpp | 12 
 4 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Preprocessor/builtin_aux_info.cpp

diff --git a/clang/include/clang/Basic/Builtins.h 
b/clang/include/clang/Basic/Builtins.h
index 63559d977ce6b6..0939f95b0922c1 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -74,6 +74,7 @@ struct Info {
   const char *Features;
   HeaderDesc Header;
   LanguageID Langs;
+  bool operator==(const Info &Other) const;
 };
 
 /// Holds information about both target-independent and
@@ -268,6 +269,10 @@ class Context {
   /// for AuxTarget).
   unsigned getAuxBuiltinID(unsigned ID) const { return ID - TSRecords.size(); }
 
+  // Return true if the AuxBuiltin ID represents a target-specific builtin that
+  // is always unsupported on the default target.
+  bool isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(unsigned ID) const;
+
   /// Returns true if this is a libc/libm function without the '__builtin_'
   /// prefix.
   static bool isBuiltinFunc(llvm::StringRef Name);
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 588183788de322..c6e54b89e848cc 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -41,6 +41,14 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 #include "clang/Basic/Builtins.inc"
 };
 
+bool Builtin::Info::operator==(const Builtin::Info &Other) const {
+  auto StrCompare = [](StringRef A, StringRef B) { return A == B; };
+  return Name == Other.Name && StrCompare(Type, Other.Type) &&
+ StrCompare(Attributes, Other.Attributes) &&
+ StrCompare(Features, Other.Features) && Header.ID == Other.Header.ID 
&&
+ Langs == Other.Langs;
+}
+
 const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
   if (ID < Builtin::FirstTSBuiltin)
 return BuiltinInfo[ID];
@@ -183,6 +191,17 @@ unsigned Builtin::Context::getRequiredVectorWidth(unsigned 
ID) const {
   return Width;
 }
 
+bool Builtin::Context::isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+unsigned ID) const {
+  assert(isAuxTargetBuiltinID(ID) && "Expected aux target builtin ID");
+  const auto &Record = getRecord(ID);
+  for (const auto &MainTargetBuiltin : TSRecords)
+if (Record == MainTargetBuiltin)
+  return false;
+
+  return true;
+}
+
 bool Builtin::Context::isLike(unsigned ID, unsigned &FormatIdx,
   bool &HasVAListArg, const char *Fmt) const {
   assert(Fmt && "Not passed a format string");
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index 347c13da0ad215..13d9a0094a5827 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1804,8 +1804,9 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
diag::err_feature_check_malformed);
 if (!II)
   return false;
-else if (II->getBuiltinID() != 0) {
-  switch (II->getBuiltinID()) {
+auto BuiltinID = II->getBuiltinID();
+if (BuiltinID != 0) {
+  switch (BuiltinID) {
   case Builtin::BI__builtin_cpu_is:
 return getTargetInfo().supportsCpuIs();
   case Builtin::BI__builtin_cpu_init:
@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+BuiltinID))
+  return false;
 return Builtin::evaluateRequiredTargetFeatures(
-

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Joseph Huber via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

jhuber6 wrote:

That seems like some weird behavior, since it's just going to end up with the 
GNU environment emitting an MSVC builtin. Maybe @boomanaiden154 can chime in on 
whether or not that's intended behavior.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex edited 
https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Nick Sarnie via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

sarnex wrote:

> Why do we need this extra logic? I figured it would simply be if the builtin 
> is only in the aux target, then we return false.

If you mean why isn't the check just `if 
(getBuiltinInfo().isAuxBuiltinID(BuiltinID))`, it's because doing that breaks 
`test/Headers/__cpuidex_conflict.c` (the last `RUN` line in the test). 

That test has x86 for both the default and aux target, but the builtin the test 
cares about is MSVC only and the default target env isn't MSVC, so that builtin 
considered unsupported and not registered, but the aux target is MSVC, so it is 
registered and `isAuxBuiltinID` returns true, and then returning false in 
`__has_builtin` breaks the test. 

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Joseph Huber via cfe-commits 


@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(

jhuber6 wrote:

Why do we need this extra logic? I figured it would simply be if the builtin is 
*only* in the aux target, then we return false.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Yaxun Liu via cfe-commits 

yxsamliu wrote:

> > I am afraid this will break all existing CUDA/HIP programs since they 
> > expect to be able to parse the builtins for both host and device targets.
> > In the spirit of single source, the compiler sees the entire code for all 
> > targets, including host target and all device targets. It is supposed to 
> > have AST for all host and device targets. Ideally, it should generate a 
> > heterogeneous IR that is for all host and device targets, and eventually 
> > generate a heterogeneous executable that includes everything. That would 
> > make some optimizations across host and device IR possible. It is just due 
> > to current limitation of LLVM/Clang, we generate and process IR separately 
> > for each host and device target.
> > I think at least during parsing/sema/AST clang could and should see code 
> > and AST for both host and device, since that helps avoid inconsistency 
> > between host and device. Therefore it is necessary to see builtins for both 
> > host and device. Clang only needs to make sure they are only emitted for 
> > the supported target.
> 
> This is/was my concern. However, upon thinking further, as long as we 
> RECOGNIZE/Parse/etc the builtins, it is OK I think if we report 
> "!__has_builtin".
> 
> That is:
> 
> ```
> void foo() {
> #if __has_builtin(__builtin_x86_thing)
> __builtin_x86_thing();
> #else
> __builtin_x86_thing(); // woops, not has_builtin!
> #endif
> }
> ```
> 
> NEEDS to still work, even if the `__has_builtin` answer is 'no'.

If declarations are still there. Only making changes to `__has_builtin`, the 
risk of breaking existing code is low. 

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Joseph Huber via cfe-commits 


@@ -0,0 +1,12 @@
+// REQUIRES: spirv-registered-target
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -fopenmp -triple=spirv64 -fopenmp-is-target-device \
+// RUN: -aux-triple x86_64-linux-unknown -E %s | FileCheck 
-implicit-check-not=BAD %s

jhuber6 wrote:

Please add lines for other targets.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Joseph Huber via cfe-commits 


@@ -0,0 +1,12 @@
+// REQUIRES: spirv-registered-target
+// REQUIRES: x86-registered-target
+

jhuber6 wrote:

These shouldn't be necessary, we're just emitting IR.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

> This is/was my concern. However, upon thinking further, as long as we 
> RECOGNIZE/Parse/etc the builtins, it is OK I think if we report 
> "!__has_builtin".
> 
> That is:
> 
> ```
> void foo() {
> #if __has_builtin(__builtin_x86_thing)
> __builtin_x86_thing();
> #else
> __builtin_x86_thing(); // woops, not has_builtin!
> #endif
> }
> ```
> 
> NEEDS to still work, even if the `__has_builtin` answer is 'no'.

Yes, I think that makes sense as it would allow both sides to parse the same 
code without forcing the compiler to always emit an incompatible builtin call. 
Since we already allow users to split compilations with preprocessor values I 
think that's reasonable.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

> I am afraid this will break all existing CUDA/HIP programs since they expect 
> to be able to parse the builtins for both host and device targets.
> 
> In the spirit of single source, the compiler sees the entire code for all 
> targets, including host target and all device targets. It is supposed to have 
> AST for all host and device targets. Ideally, it should generate a 
> heterogeneous IR that is for all host and device targets, and eventually 
> generate a heterogeneous executable that includes everything. That would make 
> some optimizations across host and device IR possible. It is just due to 
> current limitation of LLVM/Clang, we generate and process IR separately for 
> each host and device target.
> 
> I think at least during parsing/sema/AST clang could and should see code and 
> AST for both host and device, since that helps avoid inconsistency between 
> host and device. Therefore it is necessary to see builtins for both host and 
> device. Clang only needs to make sure they are only emitted for the supported 
> target.

I'm curious what would break, since I cannot imagine a single situation where 
someone would use `__has_builtin` and not immediately use said builtin. The 
current behavior makes this unusable in a device context. There's already ways 
to split the host/device compiles via the preprocessor (i.e. 
`__HIP_DEVICE_COMPILE__` or `__CUDA_ARCH`) and it's pretty much baked into the 
CUDA compilation pipeline, and by extension HIP. Other languages like OpenMP 
and SYCL skirt around the idea, seeking to put these things into the AST 
instead of the processor, so it's more of an implementation detail there.

People can already use the preprocessor to emit different code depending on the 
target architecture, I don't see how this is any different. It's important that 
both sides match up as much as possible so things like sizes of structs are 
coherent, but I really don't see this case breaking anything more than it 
already is.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Erich Keane via cfe-commits 

erichkeane wrote:

> I am afraid this will break all existing CUDA/HIP programs since they expect 
> to be able to parse the builtins for both host and device targets.
> 
> In the spirit of single source, the compiler sees the entire code for all 
> targets, including host target and all device targets. It is supposed to have 
> AST for all host and device targets. Ideally, it should generate a 
> heterogeneous IR that is for all host and device targets, and eventually 
> generate a heterogeneous executable that includes everything. That would make 
> some optimizations across host and device IR possible. It is just due to 
> current limitation of LLVM/Clang, we generate and process IR separately for 
> each host and device target.
> 
> I think at least during parsing/sema/AST clang could and should see code and 
> AST for both host and device, since that helps avoid inconsistency between 
> host and device. Therefore it is necessary to see builtins for both host and 
> device. Clang only needs to make sure they are only emitted for the supported 
> target.

This is/was my concern.  However, upon thinking further, as long as we 
RECOGNIZE/Parse/etc the builtins, it is OK I think if we report 
"!__has_builtin".

That is:
```
void foo() {
#if __has_builtin(__builtin_x86_thing)
__builtin_x86_thing();
#else
__builtin_x86_thing(); // woops, not has_builtin!
#endif
}
```

NEEDS to still work, even if the `__has_builtin` answer is 'no'.



https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-08 Thread Yaxun Liu via cfe-commits 

yxsamliu wrote:

I am afraid this will break all existing CUDA/HIP programs since they expect to 
be able to parse the builtins for both host and device targets.

In the spirit of single source, the compiler sees the entire code for all 
targets, including host target and all device targets. It is supposed to have 
AST for all host and device targets. Ideally, it should generate a 
heterogeneous IR that is for all host and device targets, and eventually 
generate a heterogeneous executable that includes everything. That would make 
some optimizations across host and device IR possible. It is just due to 
current limitation of LLVM/Clang, we generate and process IR separately for 
each host and device target.

I think at least during parsing/sema/AST clang could and should see code and 
AST for both host and device, since that helps avoid inconsistency between host 
and device. Therefore it is necessary to see builtins for both host and device. 
Clang only needs to make sure they are only emitted for the supported target.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

> > I don't think it makes any sense for `__has_builtin` to return true when 
> > the target does not in-fact have the builtin. Most of the time this is used 
> > to guard target specific code, which will then be wrong if it's compiled on 
> > the device. Realistically the solution that makes sense to me is to not 
> > emit any of the aux target's builtins as available.
> 
> From the user's perspective, the target DOES support the builtin though, 
> since from their perspective, their target is 'SYCL host + device'. So this 
> is actually not as clear as "it should never", because target/aux-target are 
> machinations of the implementation, and don't properly respect the user's 
> definition of 'target'.

I don't see how this is desirable behavior, since it just leads to stuff like 
this https://godbolt.org/z/na4rGn9od. This behavior makes `__has_builtin` 
effectively unusable if you ever use it inside a function shared with the host 
+ device.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Alex Voicu via cfe-commits 

AlexVlx wrote:

> > I don't think it makes any sense for `__has_builtin` to return true when 
> > the target does not in-fact have the builtin. Most of the time this is used 
> > to guard target specific code, which will then be wrong if it's compiled on 
> > the device. Realistically the solution that makes sense to me is to not 
> > emit any of the aux target's builtins as available.
> 
> From the user's perspective, the target DOES support the builtin though, 
> since from their perspective, their target is 'SYCL host + device'. So this 
> is actually not as clear as "it should never", because target/aux-target are 
> machinations of the implementation, and don't properly respect the user's 
> definition of 'target'.

Right, this is essentially a consequence of trying to provide the single-source 
appearance / illusion.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Erich Keane via cfe-commits 

erichkeane wrote:

> I don't think it makes any sense for `__has_builtin` to return true when the 
> target does not in-fact have the builtin. Most of the time this is used to 
> guard target specific code, which will then be wrong if it's compiled on the 
> device. Realistically the solution that makes sense to me is to not emit any 
> of the aux target's builtins as available.

>From the user's perspective, the target DOES support the builtin though, since 
>from their perspective, their target is 'SYCL host + device'.  So this is 
>actually not as clear as "it should never", because target/aux-target are 
>machinations of the implementation, and don't properly respect the user's 
>definition of 'target'.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

I don't think it makes any sense for `__has_builtin` to return true when the 
target does not in-fact have the builtin. Most of the time this is used to 
guard target specific code, which will then be wrong if it's compiled on the 
device. Realistically the solution that makes sense to me is to not emit any of 
the aux target's builtins as available.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Erich Keane via cfe-commits 

erichkeane wrote:

I think I wrote the original patch.  The point for allowing the 'aux' target to 
enable a builtin that is not supported on the 'default' target is for languages 
where 2-pass compilation is necessary for device/host, and the 'aux' target is 
used to capture that this is a 2-pass compilation.

So I think we're conflating "This should be legal in this program" and "this 
should be illegal to execute on THIS target".  Historically, those were the 
same questions, but 2 pass compilation makes this no longer true.  So right now 
this functionality is answering the 1st question, but we have no way for the 
2nd question.

IMO, we probably have something where we need to have code-gen start diagnosing 
the 2nd question.  Because something like:
```
void only_executed_on_host() {
__builtin_host_thing();
}

void device_entry_point() {
 // doesn't call executed_on_host
}

```

is a pattern that we need to support.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Nick Sarnie via cfe-commits 

sarnex wrote:

> It is not quite trying to fix a problem:). In hipstdpar mode there's no 
> host/device segregation, and it is possible to codegen IR for host only bits 
> (e.g., parts of the stdlib). We delay resolving whether a construct is or 
> isn't viable to the ME, and just place in these special stubs where the 
> otherwise unsupported construct (in this case a builtin) is used. Then, iff 
> it turns out that the code path that reaches the unsupported construct is 
> accessible, we error out - please see https://reviews.llvm.org/D155850 and 
> https://reviews.llvm.org/D155856 for historical context.
> 
> I don't quite think that this change interacts with that at all (I'll look 
> again), but it might interact in some subtle way with offload languages such 
> as e.g. HIP which jump through some hoops to keep the AST consistent, so I've 
> added @yxsamliu to this review.

Ah, thanks for the clarification for for adding reviewers!

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Alex Voicu via cfe-commits 

AlexVlx wrote:

> `CodeGenHipStdPar/unsupported-builtins.cpp` is pretty interesting actually, 
> it looks like it tests for some behavior in CodeGen that seems like it's 
> trying to fix the exact same problem
> 
> The other two tests seem to be actually unrelated breakages though.

It is not quite trying to fix a problem:). In hipstdpar mode there's no 
host/device segregation, and it is possible to codegen IR for host only bits 
(e.g., parts of the stdlib). We delay resolving whether a construct is or isn't 
viable to the ME, and just place in these special stubs where the otherwise 
unsupported construct (in this case a builtin) is used. Then, iff it turns out 
that the code path that reaches the unsupported construct is accessible, we 
error out - please see  and 
 for historical context. 

I don't quite think that this change interacts with that at all (I'll look 
again), but it might interact in some subtle way with offload languages such as 
e.g. HIP which jump through some hoops to keep the AST consistent, so I've 
added @yxsamliu to this review.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Nick Sarnie (sarnex)


Changes

Currently, `__has_builtin` will return true when passed a builtin that is only 
supported on the aux target. I found this when  `__has_builtin` was called with 
an X86 builtin but the current target was SPIR-V.

If we know for sure the builtin can't be supported on the current target, the 
function should return false.

We can't simply check if it's an aux builtin, see the test mentioned in the 
comment.

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


4 Files Affected:

- (modified) clang/include/clang/Basic/Builtins.h (+5) 
- (modified) clang/lib/Basic/Builtins.cpp (+19) 
- (modified) clang/lib/Lex/PPMacroExpansion.cpp (+17-3) 
- (added) clang/test/Preprocessor/builtin_aux_info.cpp (+12) 


``diff
diff --git a/clang/include/clang/Basic/Builtins.h 
b/clang/include/clang/Basic/Builtins.h
index 63559d977ce6b6..0939f95b0922c1 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -74,6 +74,7 @@ struct Info {
   const char *Features;
   HeaderDesc Header;
   LanguageID Langs;
+  bool operator==(const Info &Other) const;
 };
 
 /// Holds information about both target-independent and
@@ -268,6 +269,10 @@ class Context {
   /// for AuxTarget).
   unsigned getAuxBuiltinID(unsigned ID) const { return ID - TSRecords.size(); }
 
+  // Return true if the AuxBuiltin ID represents a target-specific builtin that
+  // is always unsupported on the default target.
+  bool isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(unsigned ID) const;
+
   /// Returns true if this is a libc/libm function without the '__builtin_'
   /// prefix.
   static bool isBuiltinFunc(llvm::StringRef Name);
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 588183788de322..bd443b3b371e35 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -41,6 +41,14 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 #include "clang/Basic/Builtins.inc"
 };
 
+bool Builtin::Info::operator==(const Builtin::Info &Other) const {
+  auto StrCompare = [](StringRef A, StringRef B) { return A == B; };
+  return Name == Other.Name && StrCompare(Type, Other.Type) &&
+ StrCompare(Attributes, Other.Attributes) &&
+ StrCompare(Features, Other.Features) && Header.ID == Other.Header.ID 
&&
+ Langs == Other.Langs;
+}
+
 const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
   if (ID < Builtin::FirstTSBuiltin)
 return BuiltinInfo[ID];
@@ -183,6 +191,17 @@ unsigned Builtin::Context::getRequiredVectorWidth(unsigned 
ID) const {
   return Width;
 }
 
+bool Builtin::Context::isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+unsigned ID) const {
+  assert(isAuxBuiltinID(ID) && "Expected aux target builtin ID");
+  const auto &Record = getRecord(ID);
+  for (const auto &MainTargetBuiltin : TSRecords)
+if (Record == MainTargetBuiltin)
+  return false;
+
+  return true;
+}
+
 bool Builtin::Context::isLike(unsigned ID, unsigned &FormatIdx,
   bool &HasVAListArg, const char *Fmt) const {
   assert(Fmt && "Not passed a format string");
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index 347c13da0ad215..13d9a0094a5827 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1804,8 +1804,9 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
diag::err_feature_check_malformed);
 if (!II)
   return false;
-else if (II->getBuiltinID() != 0) {
-  switch (II->getBuiltinID()) {
+auto BuiltinID = II->getBuiltinID();
+if (BuiltinID != 0) {
+  switch (BuiltinID) {
   case Builtin::BI__builtin_cpu_is:
 return getTargetInfo().supportsCpuIs();
   case Builtin::BI__builtin_cpu_init:
@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+BuiltinID))
+

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex ready_for_review 
https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-07 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex updated 
https://github.com/llvm/llvm-project/pull/121839

>From 997373350540448d91f9884b98cbdc0df058a7a3 Mon Sep 17 00:00:00 2001
From: "Sarnie, Nick" 
Date: Mon, 6 Jan 2025 11:48:07 -0800
Subject: [PATCH 1/2] [Clang] __has_builtin should return false for aux triple
 builtins

Signed-off-by: Sarnie, Nick 
---
 clang/include/clang/Basic/Builtins.h |  5 +
 clang/lib/Basic/Builtins.cpp | 19 +++
 clang/lib/Lex/PPMacroExpansion.cpp   | 20 +---
 clang/test/Preprocessor/builtin_aux_info.cpp | 12 
 4 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Preprocessor/builtin_aux_info.cpp

diff --git a/clang/include/clang/Basic/Builtins.h 
b/clang/include/clang/Basic/Builtins.h
index 63559d977ce6b6..0939f95b0922c1 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -74,6 +74,7 @@ struct Info {
   const char *Features;
   HeaderDesc Header;
   LanguageID Langs;
+  bool operator==(const Info &Other) const;
 };
 
 /// Holds information about both target-independent and
@@ -268,6 +269,10 @@ class Context {
   /// for AuxTarget).
   unsigned getAuxBuiltinID(unsigned ID) const { return ID - TSRecords.size(); }
 
+  // Return true if the AuxBuiltin ID represents a target-specific builtin that
+  // is always unsupported on the default target.
+  bool isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(unsigned ID) const;
+
   /// Returns true if this is a libc/libm function without the '__builtin_'
   /// prefix.
   static bool isBuiltinFunc(llvm::StringRef Name);
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 588183788de322..c6e54b89e848cc 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -41,6 +41,14 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 #include "clang/Basic/Builtins.inc"
 };
 
+bool Builtin::Info::operator==(const Builtin::Info &Other) const {
+  auto StrCompare = [](StringRef A, StringRef B) { return A == B; };
+  return Name == Other.Name && StrCompare(Type, Other.Type) &&
+ StrCompare(Attributes, Other.Attributes) &&
+ StrCompare(Features, Other.Features) && Header.ID == Other.Header.ID 
&&
+ Langs == Other.Langs;
+}
+
 const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
   if (ID < Builtin::FirstTSBuiltin)
 return BuiltinInfo[ID];
@@ -183,6 +191,17 @@ unsigned Builtin::Context::getRequiredVectorWidth(unsigned 
ID) const {
   return Width;
 }
 
+bool Builtin::Context::isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+unsigned ID) const {
+  assert(isAuxTargetBuiltinID(ID) && "Expected aux target builtin ID");
+  const auto &Record = getRecord(ID);
+  for (const auto &MainTargetBuiltin : TSRecords)
+if (Record == MainTargetBuiltin)
+  return false;
+
+  return true;
+}
+
 bool Builtin::Context::isLike(unsigned ID, unsigned &FormatIdx,
   bool &HasVAListArg, const char *Fmt) const {
   assert(Fmt && "Not passed a format string");
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index 347c13da0ad215..13d9a0094a5827 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1804,8 +1804,9 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
diag::err_feature_check_malformed);
 if (!II)
   return false;
-else if (II->getBuiltinID() != 0) {
-  switch (II->getBuiltinID()) {
+auto BuiltinID = II->getBuiltinID();
+if (BuiltinID != 0) {
+  switch (BuiltinID) {
   case Builtin::BI__builtin_cpu_is:
 return getTargetInfo().supportsCpuIs();
   case Builtin::BI__builtin_cpu_init:
@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is an aux builtin and it can never be supported on the default
+// target, __has_builtin should return false.
+if (getBuiltinInfo().isAuxBuiltinID(BuiltinID) &&
+
getBuiltinInfo().isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+BuiltinID))
+  return false;
 return Builtin::evaluateRequiredTargetFeatures(
-

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-06 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

> `CodeGenHipStdPar/unsupported-builtins.cpp` is pretty interesting actually, 
> it looks like it tests for some behavior in CodeGen that seems like it's 
> trying to fix the exact same problem
> 
> The other two tests seem to be actually unrelated breakages though.

Maybe @AlexVlx would know about this in general, but I'd definitely say it's 
problematic behavior.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-06 Thread Nick Sarnie via cfe-commits 

sarnex wrote:

`.CodeGenHipStdPar/unsupported-builtins.cpp` is pretty interesting actually, it 
looks like it tests for some behavior in CodeGen that seems like it's trying to 
fix the exact same problem

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-06 Thread Nick Sarnie via cfe-commits 

sarnex wrote:

> Is it possible that we could just skip generating the builtin IDs at all for 
> the aux target? Or does that break something.

Seems like we can't, I tried this:
```
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index c6e54b89e848..904c64f05c36 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -64,8 +64,8 @@ void Builtin::Context::InitializeTarget(const TargetInfo 
&Target,
 const TargetInfo *AuxTarget) {
   assert(TSRecords.empty() && "Already initialized target?");
   TSRecords = Target.getTargetBuiltins();
-  if (AuxTarget)
-AuxTSRecords = AuxTarget->getTargetBuiltins();
+  //  if (AuxTarget)
+  //  AuxTSRecords = AuxTarget->getTargetBuiltins();
 }
```
and got these failures
```
Failed Tests (3):
  Clang :: CodeGenHipStdPar/unsupported-builtins.cpp
  Clang :: Headers/__cpuidex_conflict.c
  Clang :: Sema/builtin-spirv-amdgcn-atomic-inc-dec-failure.cpp
```

and then reverted that and tried this
```
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index c6e54b89e848..34e1d0aa4b68 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -153,9 +153,9 @@ void Builtin::Context::initializeBuiltins(IdentifierTable 
&Table,
   Table.get(TSRecords[i].Name).setBuiltinID(i + Builtin::FirstTSBuiltin);
 
   // Step #3: Register target-specific builtins for AuxTarget.
-  for (unsigned i = 0, e = AuxTSRecords.size(); i != e; ++i)
-Table.get(AuxTSRecords[i].Name)
-.setBuiltinID(i + Builtin::FirstTSBuiltin + TSRecords.size());
+  //for (unsigned i = 0, e = AuxTSRecords.size(); i != e; ++i)
+  //  Table.get(AuxTSRecords[i].Name)
+  //  .setBuiltinID(i + Builtin::FirstTSBuiltin + TSRecords.size());
 
   // Step #4: Unregister any builtins specified by -fno-builtin-foo.
   for (llvm::StringRef Name : LangOpts.NoBuiltinFuncs) {

```
and got it broke the same tests
```
Failed Tests (3):
  Clang :: CodeGenHipStdPar/unsupported-builtins.cpp
  Clang :: Headers/__cpuidex_conflict.c
  Clang :: Sema/builtin-spirv-amdgcn-atomic-inc-dec-failure.cpp

```

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-06 Thread Joseph Huber via cfe-commits 

jhuber6 wrote:

Ah, I see how it is. And it probably worked in the NVPTX / AMDGCN case because 
we had valid code paths. This is definitely something that should be fixed, 
because it makes no sense to say a builtin is available if you can't even use 
it. Though offloading languages do tend to hate anything that lets users peel 
back the layers and see them as two separate compile jobs.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-06 Thread Joseph Huber via cfe-commits 

https://github.com/jhuber6 commented:

Is it possible that we could just skip generating the builtin IDs at all for 
the aux target? Or does that break something.

https://github.com/llvm/llvm-project/pull/121839
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] __has_builtin should return false for aux triple builtins (PR #121839)

2025-01-06 Thread Nick Sarnie via cfe-commits 

https://github.com/sarnex created 
https://github.com/llvm/llvm-project/pull/121839

Currently, `__has_builtin` will return true when passed a builtin that is only 
supported on the aux target. I found this when  `__has_builtin` was called with 
an X86 builtin but the current target was SPIR-V.

If we know for sure the builtin can't be supported on the current target, the 
function should return false.

We can't simply check if it's an aux builtin, see the test mentioned in the 
comment.

>From 997373350540448d91f9884b98cbdc0df058a7a3 Mon Sep 17 00:00:00 2001
From: "Sarnie, Nick" 
Date: Mon, 6 Jan 2025 11:48:07 -0800
Subject: [PATCH] [Clang] __has_builtin should return false for aux triple
 builtins

Signed-off-by: Sarnie, Nick 
---
 clang/include/clang/Basic/Builtins.h |  5 +
 clang/lib/Basic/Builtins.cpp | 19 +++
 clang/lib/Lex/PPMacroExpansion.cpp   | 20 +---
 clang/test/Preprocessor/builtin_aux_info.cpp | 12 
 4 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Preprocessor/builtin_aux_info.cpp

diff --git a/clang/include/clang/Basic/Builtins.h 
b/clang/include/clang/Basic/Builtins.h
index 63559d977ce6b6..0939f95b0922c1 100644
--- a/clang/include/clang/Basic/Builtins.h
+++ b/clang/include/clang/Basic/Builtins.h
@@ -74,6 +74,7 @@ struct Info {
   const char *Features;
   HeaderDesc Header;
   LanguageID Langs;
+  bool operator==(const Info &Other) const;
 };
 
 /// Holds information about both target-independent and
@@ -268,6 +269,10 @@ class Context {
   /// for AuxTarget).
   unsigned getAuxBuiltinID(unsigned ID) const { return ID - TSRecords.size(); }
 
+  // Return true if the AuxBuiltin ID represents a target-specific builtin that
+  // is always unsupported on the default target.
+  bool isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(unsigned ID) const;
+
   /// Returns true if this is a libc/libm function without the '__builtin_'
   /// prefix.
   static bool isBuiltinFunc(llvm::StringRef Name);
diff --git a/clang/lib/Basic/Builtins.cpp b/clang/lib/Basic/Builtins.cpp
index 588183788de322..c6e54b89e848cc 100644
--- a/clang/lib/Basic/Builtins.cpp
+++ b/clang/lib/Basic/Builtins.cpp
@@ -41,6 +41,14 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 #include "clang/Basic/Builtins.inc"
 };
 
+bool Builtin::Info::operator==(const Builtin::Info &Other) const {
+  auto StrCompare = [](StringRef A, StringRef B) { return A == B; };
+  return Name == Other.Name && StrCompare(Type, Other.Type) &&
+ StrCompare(Attributes, Other.Attributes) &&
+ StrCompare(Features, Other.Features) && Header.ID == Other.Header.ID 
&&
+ Langs == Other.Langs;
+}
+
 const Builtin::Info &Builtin::Context::getRecord(unsigned ID) const {
   if (ID < Builtin::FirstTSBuiltin)
 return BuiltinInfo[ID];
@@ -183,6 +191,17 @@ unsigned Builtin::Context::getRequiredVectorWidth(unsigned 
ID) const {
   return Width;
 }
 
+bool Builtin::Context::isAuxBuiltinIDAlwaysUnsupportedOnDefaultTarget(
+unsigned ID) const {
+  assert(isAuxTargetBuiltinID(ID) && "Expected aux target builtin ID");
+  const auto &Record = getRecord(ID);
+  for (const auto &MainTargetBuiltin : TSRecords)
+if (Record == MainTargetBuiltin)
+  return false;
+
+  return true;
+}
+
 bool Builtin::Context::isLike(unsigned ID, unsigned &FormatIdx,
   bool &HasVAListArg, const char *Fmt) const {
   assert(Fmt && "Not passed a format string");
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index 347c13da0ad215..13d9a0094a5827 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1804,8 +1804,9 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
diag::err_feature_check_malformed);
 if (!II)
   return false;
-else if (II->getBuiltinID() != 0) {
-  switch (II->getBuiltinID()) {
+auto BuiltinID = II->getBuiltinID();
+if (BuiltinID != 0) {
+  switch (BuiltinID) {
   case Builtin::BI__builtin_cpu_is:
 return getTargetInfo().supportsCpuIs();
   case Builtin::BI__builtin_cpu_init:
@@ -1818,8 +1819,21 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
 // usual allocation and deallocation functions. Required by libc++
 return 201802;
   default:
+// We may get here because of aux builtins which may not be
+// supported on the default target, for example if we have an X86
+// specific builtin and the current target is SPIR-V. Sometimes we
+// rely on __has_builtin returning true when passed a builtin that
+// is not supported on the default target due to LangOpts but is
+// supported on the aux target. See
+// test/Headers/__cpuidex_conflict.c for an example. If the builtin
+// is