-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Clang] Add __has_target_builtin macro #126324
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
46cce74
2a2a18b
644ea85
887de75
7781516
5d84bc5
7e2dc73
2668404
7143389
6682ae2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -67,6 +67,11 @@ It can be used like this: | |||||
``__has_builtin`` should not be used to detect support for a builtin macro; | ||||||
use ``#ifdef`` instead. | ||||||
|
||||||
When using device offloading, a builtin is considered available if it is | ||||||
available on either the host or the device targets. | ||||||
Use ``__has_target_builtin`` to consider only the current target for an | ||||||
offloading target. | ||||||
|
||||||
``__has_constexpr_builtin`` | ||||||
--------------------------- | ||||||
|
||||||
|
@@ -96,6 +101,37 @@ the ``<cmath>`` header file to conditionally make a function constexpr whenever | |||||
the constant evaluation of the corresponding builtin (for example, | ||||||
``std::fmax`` calls ``__builtin_fmax``) is supported in Clang. | ||||||
|
||||||
``__has_target_builtin`` | ||||||
------------------------ | ||||||
|
||||||
This function-like macro takes a single identifier argument that is the name of | ||||||
a builtin function, a builtin pseudo-function (taking one or more type | ||||||
arguments), or a builtin template. | ||||||
It evaluates to 1 if the builtin is supported on the current target or 0 if not. | ||||||
The behavior is different than ``__has_builtin`` when there is an auxiliary target, | ||||||
such when offloading to a target device. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rephrase it to be more specific in terms of what the difference is rather than when it occurs.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, i like the way you worded it so i'll use most of this verbatim |
||||||
It can be used like this: | ||||||
|
||||||
.. code-block:: c++ | ||||||
|
||||||
#ifndef __has_target_builtin // Optional of course. | ||||||
#define __has_target_builtin(x) 0 // Compatibility with non-clang compilers. | ||||||
#endif | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be more helpful to do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hopefully the latest commit has the use case youre looking for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we make it available to C++, we'd better document the following invalid usage which originally leads to this extension:
we should emphasize that a C++ header may be used by offloading languages, and in offloading language, the same source is compiled for host and device target separately. A builtin not available for the current target does not justify defining the builtin for both host and device targets. In this case, better to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have somewhat conflicting requirements:
I'm not sure we can find a universal solution. That said, |
||||||
|
||||||
... | ||||||
#if __has_target_builtin(__builtin_trap) | ||||||
__builtin_trap(); | ||||||
#else | ||||||
abort(); | ||||||
#endif | ||||||
... | ||||||
|
||||||
.. note:: | ||||||
``__has_target_builtin`` should not be used to detect support for a builtin macro; | ||||||
use ``#ifdef`` instead. | ||||||
|
||||||
``__has_target_built`` is only defined for offloading targets. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow thats embarrassing, thanks |
||||||
|
||||||
.. _langext-__has_feature-__has_extension: | ||||||
|
||||||
``__has_feature`` and ``__has_extension`` | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -357,6 +357,12 @@ void Preprocessor::RegisterBuiltinMacros() { | |
Ident__has_builtin = RegisterBuiltinMacro("__has_builtin"); | ||
Ident__has_constexpr_builtin = | ||
RegisterBuiltinMacro("__has_constexpr_builtin"); | ||
if (getLangOpts().OpenMPIsTargetDevice || getLangOpts().CUDAIsDevice || | ||
getLangOpts().SYCLIsDevice) | ||
Ident__has_target_builtin = RegisterBuiltinMacro("__has_target_builtin"); | ||
else | ||
Ident__has_target_builtin = nullptr; | ||
|
||
Ident__has_attribute = RegisterBuiltinMacro("__has_attribute"); | ||
if (!getLangOpts().CPlusPlus) | ||
Ident__has_c_attribute = RegisterBuiltinMacro("__has_c_attribute"); | ||
|
@@ -1797,16 +1803,18 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) { | |
diag::err_feature_check_malformed); | ||
return II && HasExtension(*this, II->getName()); | ||
}); | ||
} else if (II == Ident__has_builtin) { | ||
yxsamliu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else if (II == Ident__has_builtin || II == Ident__has_target_builtin) { | ||
AaronBallman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool IsHasTargetBuiltin = II == Ident__has_target_builtin; | ||
EvaluateFeatureLikeBuiltinMacro( | ||
OS, Tok, II, *this, false, | ||
[this](Token &Tok, bool &HasLexedNextToken) -> int { | ||
[this, IsHasTargetBuiltin](Token &Tok, bool &HasLexedNextToken) -> int { | ||
IdentifierInfo *II = ExpectFeatureIdentifierInfo( | ||
Tok, *this, diag::err_feature_check_malformed); | ||
if (!II) | ||
return false; | ||
else if (II->getBuiltinID() != 0) { | ||
switch (II->getBuiltinID()) { | ||
auto BuiltinID = II->getBuiltinID(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please spell out the type explicitly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in latest commit, thx |
||
if (BuiltinID != 0) { | ||
switch (BuiltinID) { | ||
case Builtin::BI__builtin_cpu_is: | ||
return getTargetInfo().supportsCpuIs(); | ||
case Builtin::BI__builtin_cpu_init: | ||
|
@@ -1819,8 +1827,12 @@ void Preprocessor::ExpandBuiltinMacro(Token &Tok) { | |
// usual allocation and deallocation functions. Required by libc++ | ||
return 201802; | ||
default: | ||
// __has_target_builtin should return false for aux builtins. | ||
if (IsHasTargetBuiltin && | ||
getBuiltinInfo().isAuxBuiltinID(BuiltinID)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic doesn't seem right to me. What happens if this builtin is supported on BOTH the host and device here? Shouldn't we still return 'true'? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. The function name is
When we register builtins, we do it like this:
So we register all the target builtins before the aux target builtins, and we only consider something an aux builtin if it was registered specifically as an aux builtin, so I think the logic does what we want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! Neat! Can you document that somewhere on this? Just extending the comment here will be really helpful to the next person along. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, opened PR here and added you as a reviewer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! Thanks! I meant here as well though, it was REALLY jarring to me and I suspect next one here will ahve the same problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah sure, will update this pr as well |
||
return false; | ||
return Builtin::evaluateRequiredTargetFeatures( | ||
getBuiltinInfo().getRequiredFeatures(II->getBuiltinID()), | ||
getBuiltinInfo().getRequiredFeatures(BuiltinID), | ||
getTargetInfo().getTargetOpts().FeatureMap); | ||
} | ||
return true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// RUN: %clang_cc1 -fopenmp -triple=spirv64 -fopenmp-is-target-device \ | ||
// RUN: -aux-triple x86_64-linux-unknown -E %s | FileCheck -implicit-check-not="{{BAD|DOESNT}}" %s | ||
|
||
// RUN: %clang_cc1 -fopenmp -triple=nvptx64 -fopenmp-is-target-device \ | ||
// RUN: -aux-triple x86_64-linux-unknown -E %s | FileCheck -implicit-check-not="{{BAD|DOESNT}}" %s | ||
|
||
// RUN: %clang_cc1 -fopenmp -triple=amdgcn-amd-amdhsa -fopenmp-is-target-device \ | ||
// RUN: -aux-triple x86_64-linux-unknown -E %s | FileCheck -implicit-check-not="{{BAD|DOESNT}}" %s | ||
|
||
// RUN: %clang_cc1 -fopenmp -triple=aarch64 -fopenmp-is-target-device \ | ||
// RUN: -aux-triple x86_64-linux-unknown -E %s | FileCheck -implicit-check-not="{{BAD|DOESNT}}" %s | ||
|
||
// RUN: %clang_cc1 -triple=aarch64 -E %s | FileCheck -check-prefix=CHECK-NOTOFFLOAD -implicit-check-not="{{GOOD|HAS|BAD}}" %s | ||
|
||
// CHECK: GOOD | ||
|
||
// CHECK-NOTOFFLOAD: DOESNT | ||
#ifdef __has_target_builtin | ||
HAS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should probably check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the macro is unconditionally available in the latest commit so i removed the checking for the macro being defined |
||
#if __has_target_builtin(__builtin_ia32_pause) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add test coverage for when the target does have the builtin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in latest commit, thx |
||
BAD | ||
#else | ||
GOOD | ||
#endif | ||
#else | ||
DOESNT | ||
#endif |
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.
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.
cant english today, thanks