-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang] Add clang::nooutline Attribute #163666
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: users/lenary/llvm-nooutline-enum-attr
Are you sure you want to change the base?
[clang] Add clang::nooutline Attribute #163666
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Sam Elliott (lenary) ChangesThis change:
The Full diff: https://github.com/llvm/llvm-project/pull/163666.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 22e60aa9fe312..b8a61ba4cbac9 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2355,6 +2355,13 @@ def NoInline : DeclOrStmtAttr {
let SimpleHandler = 1;
}
+def NoOutline : DeclOrStmtAttr {
+ let Spellings = [CXX11<"clang", "nooutline">, C23<"clang", "nooutline">];
+ let Subjects = SubjectList<[Function], ErrorDiag>;
+ let Documentation = [Undocumented];
+ let SimpleHandler = 1;
+}
+
def NoMips16 : InheritableAttr, TargetSpecificAttr<TargetMips32> {
let Spellings = [GCC<"nomips16">];
let Subjects = SubjectList<[Function], ErrorDiag>;
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 8d019d4b2da25..ab267236ed579 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2820,6 +2820,9 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
B.addAttribute(llvm::Attribute::MinSize);
}
+ if (D->hasAttr<NoOutlineAttr>())
+ B.addAttribute(llvm::Attribute::NoOutline);
+
F->addFnAttrs(B);
unsigned alignment = D->getMaxAlignment() / Context.getCharWidth();
diff --git a/clang/test/CodeGen/attr-nooutline.c b/clang/test/CodeGen/attr-nooutline.c
new file mode 100644
index 0000000000000..b9f175da24cb5
--- /dev/null
+++ b/clang/test/CodeGen/attr-nooutline.c
@@ -0,0 +1,16 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --version 6
+// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -disable-O0-optnone -o - | FileCheck %s
+
+
+// CHECK: Function Attrs: noinline nooutline nounwind
+// CHECK-LABEL: define dso_local i32 @t1(
+// CHECK-SAME: i32 noundef [[X:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: [[X_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT: store i32 [[X]], ptr [[X_ADDR]], align 4
+// CHECK-NEXT: [[TMP0:%.*]] = load i32, ptr [[X_ADDR]], align 4
+// CHECK-NEXT: ret i32 [[TMP0]]
+//
+[[clang::nooutline]] int t1(int x) {
+ return x;
+}
diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
index 73d4cb1769ed5..7ef758dbea9eb 100644
--- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -123,6 +123,7 @@
// CHECK-NEXT: NoMerge (SubjectMatchRule_function, SubjectMatchRule_variable)
// CHECK-NEXT: NoMicroMips (SubjectMatchRule_function)
// CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
+// CHECK-NEXT: NoOutline (SubjectMatchRule_function)
// CHECK-NEXT: NoProfileFunction (SubjectMatchRule_function)
// CHECK-NEXT: NoRandomizeLayout (SubjectMatchRule_record)
// CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
diff --git a/clang/test/Sema/attr-nooutline.c b/clang/test/Sema/attr-nooutline.c
new file mode 100644
index 0000000000000..83848c22d1003
--- /dev/null
+++ b/clang/test/Sema/attr-nooutline.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+[[clang::nooutline]] int a; // expected-error {{'clang::nooutline' attribute only applies to functions}}
+
+[[clang::nooutline]] void t1(void);
+
+[[clang::nooutline(2)]] void t2(void); // expected-error {{'clang::nooutline' attribute takes no arguments}}
diff --git a/clang/test/Sema/attr-nooutline.cpp b/clang/test/Sema/attr-nooutline.cpp
new file mode 100644
index 0000000000000..b6c9b3995081a
--- /dev/null
+++ b/clang/test/Sema/attr-nooutline.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s -Wno-c++17-extensions
+
+[[clang::nooutline]] int a; // expected-error {{'clang::nooutline' attribute only applies to functions}}
+
+[[clang::nooutline]] void t1(void);
+
+[[clang::nooutline(2)]] void t2(void); // expected-error {{'clang::nooutline' attribute takes no arguments}}
|
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.
General comment: I really dislike the spelling. The first time I read this I interpreted it as "noout-line" not "no-outline".
clang/include/clang/Basic/Attr.td
Outdated
def NoOutline : DeclOrStmtAttr { | ||
let Spellings = [CXX11<"clang", "nooutline">, C23<"clang", "nooutline">]; | ||
let Subjects = SubjectList<[Function], ErrorDiag>; | ||
let Documentation = [Undocumented]; |
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.
New attributes shouldn't be undocumented.
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.
I have added documentation.
clang/include/clang/Basic/Attr.td
Outdated
} | ||
|
||
def NoOutline : DeclOrStmtAttr { | ||
let Spellings = [CXX11<"clang", "nooutline">, C23<"clang", "nooutline">]; |
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.
I don't think this is done anywhere else.
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.
NoInline
does this too but I presume that’s because it’s old; it should also be updated to use Clang<>
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.
Sorry, yes I was following NoInline
. fixed.
clang/include/clang/Basic/Attr.td
Outdated
} | ||
|
||
def NoOutline : DeclOrStmtAttr { | ||
let Spellings = [CXX11<"clang", "nooutline">, C23<"clang", "nooutline">]; |
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.
let Spellings = [CXX11<"clang", "nooutline">, C23<"clang", "nooutline">]; | |
let Spellings = [Clang<"nooutline">]; |
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.
Done, thanks for the pointer.
clang/include/clang/Basic/Attr.td
Outdated
} | ||
|
||
def NoOutline : DeclOrStmtAttr { | ||
let Spellings = [CXX11<"clang", "nooutline">, C23<"clang", "nooutline">]; |
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.
NoInline
does this too but I presume that’s because it’s old; it should also be updated to use Clang<>
clang/test/CodeGen/attr-nooutline.c
Outdated
@@ -0,0 +1,16 @@ | |||
// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --version 6 | |||
// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -disable-O0-optnone -o - | FileCheck %s |
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.
We should also have a C++ codegen test, but that can just be an extra RUN line w/ -x c++
.
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.
Done.
I don't know if there's something I'm missing by using Function, which I should be testing.
For instance:
- C++ Lambdas?
- C++ Methods?
- Blocks?
- Objective-C Methods?
I presume C++ Methods will work with this, the others I'm less sure of.
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.
- C++ Lambdas?
- C++ Methods?
- Blocks?
- Objective-C Methods?
It would be a good idea to add tests for all of those yes.
Compounds with the same vowel to the left and right of a word boundary always look a bit weird yeah; that said, I personally don’t think it’s too bad; the naming is just a bit unfortunate. |
Oh yeah, there should also be a release note that mentions that we’re adding a new attribute. |
Thanks for the reviews. They're very helpful. Working on those changes now.
I went through other Clang attributes with similar Clang attributes starting "no" without punctuation to split words:
Clang attributes starting "no" with punctuation to split words:
To me this split is pretty even between with an underscore and without, but I would prefer the symmetry with |
This change: - Adds a `[[clang::nooutline]]` function attribute for C and C++. There is no equivalent GNU syntax for this attribute, so no `__attribute__` syntax. - Uses the presence of `[[clang::nooutline]]` to add the `nooutline` attribute to IR function definitions. - Adds test for the above. The `nooutline` attribute disables both the Machine Outliner (enabled at Oz for some targets), and the IR Outliner (disabled by default).
75103d1
to
a84c645
Compare
358b089
to
a729b26
Compare
- Added a new attribute, ``[[clang::nooutline]]`` to suppress outlining from | ||
annotated functions. This uses the LLVM `nooutline` attribute. |
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 is a dedicated section called ‘Attribute Changes in Clang’ below
@@ -0,0 +1,7 @@ | |||
// RUN: %clang_cc1 -verify -fsyntax-only %s -Wno-c++17-extensions |
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.
// RUN: %clang_cc1 -verify -fsyntax-only %s -Wno-c++17-extensions | |
// RUN: %clang_cc1 -verify -fsyntax-only %s |
I don’t think we need to pass -Wno-c++17-extensions
here since we’re not using any from what I can tell (and Clang defaults to C++17 anyway).
} | ||
|
||
def NoOutline : DeclOrStmtAttr { | ||
let Spellings = [Clang<"nooutline">]; |
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.
let Spellings = [Clang<"nooutline">]; | |
let Spellings = [Clang<"no_outline">]; |
perhaps? Makes it... somewhat consistent with the force_inline
/etc type attributes, and makes it a better mental 'parse'.
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.
I wrote a longer comment about this, see above.
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.
I see, thanks.
Hmm... my mental parse is still not really picking up the actual meaning of what this is (and as I'm the second one to mention that, perhaps something we should care about?). The rest don't have that parse problem for me. While I appreciate the symmetry, at the same time readability should, IMO trump it.
} | ||
|
||
}]; | ||
} |
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.
Can you explain why one would not want this to happen? Perhaps a quick example? And can you show a psuedo-code version of x2/x3 (though I'd probably suggest only a single-one of those examples, they aren't unique enough to have 2) as to what it might look like as an 'outlined' version?
// C: Function Attrs: noinline nooutline nounwind optnone | ||
// C-LABEL: define dso_local i32 @t1( | ||
// C-SAME: i32 noundef [[X:%.*]]) #[[ATTR0:[0-9]+]] { | ||
// C-NEXT: [[ENTRY:.*:]] |
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.
The body in these tests are probably not worth it since all we care about are the FunctionAttrs line and the one with the declaration. The rest seems fragile for no value.
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.
I agree with the fragility aspect, is there a better way to automatically test these or should I just write the checks by hand?
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.
These are short/easy enough I'd suggest just writing them by hand (or just deleting everything in the body of the function manually).
|
||
def NoOutline : DeclOrStmtAttr { | ||
let Spellings = [Clang<"nooutline">]; | ||
let Subjects = SubjectList<[Function], ErrorDiag>; |
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.
Your examples all use prototyped functions, is there a reason this cant be used on a no-prototype function? If so, this should reflect that (alternatively, write a codegen test to show its lack of importance).
I'm really not a fan of that. I don't think |
This change:
[[clang::nooutline]]
function attribute for C and C++. Thereis no equivalent GNU syntax for this attribute, so no
__attribute__
syntax.
[[clang::nooutline]]
to add thenooutline
attribute to IR function definitions.
The
nooutline
attribute disables both the Machine Outliner (enabled atOz for some targets), and the IR Outliner (disabled by default).