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][X86] Support __attribute__((model("small"/"large"))) #124834

Merged
merged 13 commits into from
Feb 15, 2025
+152 −58
Merged

[clang][X86] Support __attribute__((model("small"/"large"))) #124834

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7c40169
[clang][X86] Support __attribute__((model("small"/"large")))
aeubanks Jan 28, 2025
8aa6b3f
address comments
aeubanks Jan 29, 2025
2fdf2ee
add nvptx RUN line to test, refactor test
aeubanks Jan 29, 2025
1a32c31
add more flags to RUN line
aeubanks Jan 29, 2025
7ce06bd
allow and ignore attribute on nvptx compiles
aeubanks Feb 3, 2025
526e8c3
Merge remote-tracking branch 'upstream/main' into model
aeubanks Feb 3, 2025
6779705
ignore amdgpu/spirv
aeubanks Feb 4, 2025
2263db1
update AttrDocs.td
aeubanks Feb 5, 2025
bf8f7d4
more descriptive docs (at least for x86-64)
aeubanks Feb 5, 2025
d314c8e
warn if host triple doesn't support attribute
aeubanks Feb 12, 2025
1729029
llvm::Triple&
aeubanks Feb 12, 2025
c664880
one more &
aeubanks Feb 12, 2025
f3ecb7b
Merge remote-tracking branch 'upstream/main' into model
aeubanks Feb 14, 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/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,11 @@ Attribute Changes in Clang

- Clang now disallows the use of attributes after the namespace name. (#GH121407)

- Clang now allows ``__attribute__((model("small")))`` and
``__attribute__((model("large")))`` on non-TLS globals in x86-64 compilations.
This forces the global to be considered small or large in regards to the
x86-64 code model, regardless of the code model specified for the compilation.

Improvements to Clang's diagnostics
-----------------------------------

Expand Down
13 changes: 9 additions & 4 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ def TargetMSP430 : TargetArch<["msp430"]>;
def TargetM68k : TargetArch<["m68k"]>;
def TargetRISCV : TargetArch<["riscv32", "riscv64"]>;
def TargetX86 : TargetArch<["x86"]>;
def TargetX86_64 : TargetArch<["x86_64"]>;
def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
def TargetWebAssembly : TargetArch<["wasm32", "wasm64"]>;
def TargetNVPTX : TargetArch<["nvptx", "nvptx64"]>;
Expand Down Expand Up @@ -3117,11 +3118,15 @@ def PragmaClangTextSection : InheritableAttr {
let Documentation = [InternalOnly];
}

def CodeModel : InheritableAttr, TargetSpecificAttr<TargetLoongArch> {
def CodeModel : InheritableAttr,
TargetSpecificAttr<TargetArch<!listconcat(
TargetLoongArch.Arches, TargetX86_64.Arches)>> {
let Spellings = [GCC<"model">];
let Args = [EnumArgument<"Model", "llvm::CodeModel::Model", /*is_string=*/1,
["normal", "medium", "extreme"], ["Small", "Medium", "Large"],
/*opt=*/0, /*fake=*/0, /*isExternalType=*/1, /*isCovered=*/0>];
let Args = [EnumArgument<
"Model", "llvm::CodeModel::Model",
/*is_string=*/1, ["small", "normal", "medium", "large", "extreme"],
["Small", "Small", "Medium", "Large", "Large"],
/*opt=*/0, /*fake=*/0, /*isExternalType=*/1, /*isCovered=*/0>];
let Subjects = SubjectList<[NonTLSGlobalVar], ErrorDiag>;
let Documentation = [CodeModelDocs];
}
Expand Down
15 changes: 14 additions & 1 deletion clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
#include "llvm/IR/DerivedTypes.h"
#include "llvm/MC/MCSectionMachO.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/Triple.h"
Expand Down Expand Up @@ -2949,6 +2950,17 @@ static void handleSectionAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
}
}

static bool isValidCodeModelAttr(Sema &S, StringRef Str) {
if (S.Context.getTargetInfo().getTriple().isLoongArch()) {
return Str == "normal" || Str == "medium" || Str == "extreme";
} else {
assert(S.Context.getTargetInfo().getTriple().getArch() ==
llvm::Triple::x86_64 &&
"only loongarch/x86-64 supported");
return Str == "small" || Str == "large";
}
}

static void handleCodeModelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
StringRef Str;
SourceLocation LiteralLoc;
Expand All @@ -2957,7 +2969,8 @@ static void handleCodeModelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
return;

llvm::CodeModel::Model CM;
if (!CodeModelAttr::ConvertStrToModel(Str, CM)) {
if (!CodeModelAttr::ConvertStrToModel(Str, CM) ||
!isValidCodeModelAttr(S, Str)) {
S.Diag(LiteralLoc, diag::err_attr_codemodel_arg) << Str;
return;
}
Expand Down
27 changes: 27 additions & 0 deletions clang/test/CodeGen/X86/codemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// RUN: %clang_cc1 -emit-llvm -triple x86_64-unknown-unknown %s -o - | FileCheck %s

// CHECK: @_ZL2v1 ={{.*}} global i32 0, code_model "small"
static int v1 __attribute__((model("small")));

void use1() {
v1 = 1;
}

// CHECK: @v2 ={{.*}} global float 0.000000e+00, code_model "large"
float v2 __attribute__((model("large")));

// CHECK: @_ZL2v3IiE ={{.*}} global i32 0, code_model "small"
template <typename T>
static T v3 __attribute__((model("small")));

void use2() {
v3<int> = 1;
}
struct S {
double d;
};

typedef void (*F)();

// CHECK: @v4 ={{.*}} global ptr null, code_model "large"
F v4 __attribute__((model("large")));
82 changes: 29 additions & 53 deletions clang/test/Sema/attr-model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,64 +1,40 @@
// RUN: %clang_cc1 -triple aarch64 -verify=expected,aarch64 -fsyntax-only %s
// RUN: %clang_cc1 -triple aarch64 -verify=expected,unsupported -fsyntax-only %s
// RUN: %clang_cc1 -triple loongarch64 -verify=expected,loongarch64 -fsyntax-only %s
// RUN: %clang_cc1 -triple mips64 -verify=expected,mips64 -fsyntax-only %s
// RUN: %clang_cc1 -triple powerpc64 -verify=expected,powerpc64 -fsyntax-only %s
// RUN: %clang_cc1 -triple riscv64 -verify=expected,riscv64 -fsyntax-only %s
// RUN: %clang_cc1 -triple mips64 -verify=expected,unsupported -fsyntax-only %s
// RUN: %clang_cc1 -triple powerpc64 -verify=expected,unsupported -fsyntax-only %s
// RUN: %clang_cc1 -triple riscv64 -verify=expected,unsupported -fsyntax-only %s
// RUN: %clang_cc1 -triple x86_64 -verify=expected,x86_64 -fsyntax-only %s
aeubanks marked this conversation as resolved.
Show resolved Hide resolved
// RUN: %clang_cc1 -triple nvptx64-unknown-cuda -fcuda-is-device -x cuda -verify=expected,unsupported -fsyntax-only %s
Copy link
Member

Choose a reason for hiding this comment

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

If the test passes this way (I.e. you do see complaints about the attributes) that means that the change will break CUDA.

Considering that we will see these attributes in the host code and that NVPTX itself can't do anything useful with them, NVPTX compilation should continue to work and ignore those attributes.

Copy link
Member

Choose a reason for hiding this comment

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

The attribute is technically is ignored, with a warning, but the warning is likely non-actionable as it would be issues for a perfectly correct host code which just happens to be seen by a GPU compilation.

I think the correct way to handle it would be to issue a deferred diagnostic which will fire only if we end up code-gen-ing something that uses the attribute.

@yxsamliu Sam, does that make sense?

Copy link
Contributor Author

@aeubanks aeubanks Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see, although it seems pretty annoying to have to special case NVPTX compilations. Is there a previous example of having done something like this, or should I just ad-hoc skip the Sema code for NVPTX compilations?

(oops comment race)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the correct way to handle it would be to issue a deferred diagnostic which will fire only if we end up code-gen-ing something that uses the attribute.

is it common to define a global in a TU that's shared between the host and device?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. global scope is implicitly considered to belong to host. Data that's supposed to reside on the device must be explicitly annotated with appropriate attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm tempted to keep things as is since it seems like this is not the only issue that can come up with nvptx compiles. For example, the thread_local global in the variable has the same issue where nvptx doesn't support thread local globals, which clang complains about, and I don't really see a difference between __attribute__((model())) and thread_local in that they can both be used on arbitrary globals.

Do you think it's reasonable to postpone this suggestion until people actually hit it? For example, if people aren't hitting the thread_local issue, then perhaps people won't hit a warning with this attribute.

Copy link
Member

Choose a reason for hiding this comment

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

Up to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The attribute is technically is ignored, with a warning, but the warning is likely non-actionable as it would be issues for a perfectly correct host code which just happens to be seen by a GPU compilation.

I think the correct way to handle it would be to issue a deferred diagnostic which will fire only if we end up code-gen-ing something that uses the attribute.

@yxsamliu Sam, does that make sense?

My understanding is that deferred diagnostics only works in a function, but this diagnostic happens outside functions.


#if defined(__loongarch__) && !__has_attribute(model)
#if (defined(__loongarch__) || defined(__x86_64__)) && !__has_attribute(model)
#error "Should support model attribute"
#endif

int a __attribute((model("tiny"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
int a __attribute((model("tiny"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{code model 'tiny' is not supported on this target}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
int b __attribute((model("small"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{code model 'small' is not supported on this target}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
int c __attribute((model("normal"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
int d __attribute((model("kernel"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// x86_64-error {{code model 'tiny' is not supported on this target}}
int b __attribute((model("small"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{code model 'small' is not supported on this target}}
int c __attribute((model("normal"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// x86_64-error {{code model 'normal' is not supported on this target}}
int d __attribute((model("kernel"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{code model 'kernel' is not supported on this target}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
int e __attribute((model("medium"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
int f __attribute((model("large"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{code model 'large' is not supported on this target}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
int g __attribute((model("extreme"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
// x86_64-error {{code model 'kernel' is not supported on this target}}
int e __attribute((model("medium"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// x86_64-error {{code model 'medium' is not supported on this target}}
int f __attribute((model("large"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{code model 'large' is not supported on this target}}
int g __attribute((model("extreme"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// x86_64-error {{code model 'extreme' is not supported on this target}}

void __attribute((model("extreme"))) h() {} // aarch64-warning {{unknown attribute 'model' ignored}} \
void __attribute((model("extreme"))) h() {} // unsupported-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{'model' attribute only applies to non-TLS global variables}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
// x86_64-error {{'model' attribute only applies to non-TLS global variables}}

thread_local int i __attribute((model("extreme"))); // aarch64-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{'model' attribute only applies to non-TLS global variables}} \
// mips64-warning {{unknown attribute 'model' ignored}} \
// powerpc64-warning {{unknown attribute 'model' ignored}} \
// riscv64-warning {{unknown attribute 'model' ignored}} \
// x86_64-warning {{unknown attribute 'model' ignored}}
// NVPTX doesn't support thread_local at all.
#ifndef __NVPTX__
thread_local
#endif
int i __attribute((model("extreme"))); // unsupported-warning {{unknown attribute 'model' ignored}} \
// loongarch64-error {{'model' attribute only applies to non-TLS global variables}} \
// x86_64-error {{'model' attribute only applies to non-TLS global variables}}