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.

Sign up for GitHub

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

[Clang] __has_builtin should return false for aux triple builtins #121839

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+29 −4
Open

[Clang] __has_builtin should return false for aux triple builtins #121839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9973733
[Clang] __has_builtin should return false for aux triple builtins
sarnex Jan 6, 2025
55c6d06
Fix build
sarnex Jan 7, 2025
3ce5111
address lit test feedback
sarnex Jan 8, 2025
69198ca
check only for aux builtins, expect fail in failing test
sarnex Jan 22, 2025
ef1b0ee
Merge remote-tracking branch 'upstream/main'
sarnex Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/Builtins.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
19 changes: 19 additions & 0 deletions clang/lib/Basic/Builtins.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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");
Expand Down
20 changes: 17 additions & 3 deletions clang/lib/Lex/PPMacroExpansion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@sarnex sarnex Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 is 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is related to the openmp test in __cpuidex_conflict.c? I believe that is related to the following comment from @AaronBallman in https://reviews.llvm.org/D150646:

I think these changes exacerbate an issue that we had but didn't know about, because I'm able to reproduce the redefinition problem in both Clang 16 and trunk: https://godbolt.org/z/8Yq1vzEnT

What's happening is that the aux-triple is not being honored when compiling for an offload device (OpenMP, SYCL, etc) and so the wrong builtins are being enabled, leading to exposing __cpuidex as a builtin on non-Windows targets. This means the workaround to look for _MSC_EXTENSIONS doesn't solve the underlying issue -- we already try to only expose the intrinsic when MS extensions are enabled:

llvm-project/clang/include/clang/Basic/BuiltinsX86.def

Line 2098 in 40f3708

TARGET_HEADER_BUILTIN(__cpuidex, "vi*ii", "nh", INTRIN_H, ALL_MS_LANGUAGES, "")
and

llvm-project/clang/lib/Basic/Builtins.cpp

Line 91 in 40f3708

if (!LangOpts.MicrosoftExt && (BuiltinInfo.Langs & MS_LANG))

The changes in this patch are causing us problems with our downstream at Intel. I think we might want to consider reverting the change temporarily (and on the 17.x branch) because they're exacerbating an existing problem. That gives us time to solve the problem of exposing the wrong set of builtins, and hopefully means we can remove the _MSC_EXTENSIONS workaround in the header file. WDYT?

The test is intended to make sure that the __cpuidex definiton from cpuid.h isn't used (and thus doesn't conflct) with the MSVC builtin definition. I'm not really familiar enough with OpenMP to tell if I set up the test correctly, and it could very well be the test that needs to be adjusted.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't expect the aux triple part of the test to break any actual users, so that should be fine. Nothing in cpuid.h makes sense to use on an offloading target and I wouldn't really expect anyone to have done so.

Then again, I wouldn't have expected people to include cpuid.h when compiling for windows, but I can see that being a vastly more popular configuration than somehow trying to use MS builtins on code that gets offloaded.

Copy link
Member Author

@sarnex sarnex Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, are you okay with me disabling that part of the test and leaving it to you to figure out the correct guard when you have time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should work for me. Can you add a TODO(boomanaiden154) as a comment when you disable it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, thanks!

BuiltinID))
return false;
return Builtin::evaluateRequiredTargetFeatures(
getBuiltinInfo().getRequiredFeatures(II->getBuiltinID()),
getBuiltinInfo().getRequiredFeatures(BuiltinID),
getTargetInfo().getTargetOpts().FeatureMap);
}
return true;
Expand Down
12 changes: 12 additions & 0 deletions clang/test/Preprocessor/builtin_aux_info.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// REQUIRES: spirv-registered-target
// REQUIRES: x86-registered-target

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 3ce5111, thx

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add lines for other targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

hopefully addressed in 3ce5111, thx


// CHECK: GOOD
#if __has_builtin(__builtin_ia32_pause)
BAD
#else
GOOD
#endif
Loading