[Clang] Add support for [[msvc::noinline]] attribute. #91720

simonzgx · 2024-05-10T09:44:55Z

Fixes #90941.
Add support for [[msvc::noinline]] attribute, which is actually an
alias of [[clang::noinline]].

llvmbot · 2024-05-10T09:45:25Z

@llvm/pr-subscribers-clang

Author: Xu Zhang (simonzgx)

Changes

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

3 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+4-1)
(added) clang/test/CodeGen/attr-ms-noinline.cpp (+53)
(added) clang/test/Sema/attr-ms-noinline.cpp (+66)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 0225598cbbe8a..2c7a238f61764 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1997,9 +1997,12 @@ def Convergent : InheritableAttr {
 def NoInline : DeclOrStmtAttr {
   let Spellings = [CustomKeyword<"__noinline__">, GCC<"noinline">,
                    CXX11<"clang", "noinline">, C23<"clang", "noinline">,
+                   CXX11<"msvc", "noinline">, C23<"msvc", "noinline">,
                    Declspec<"noinline">];
   let Accessors = [Accessor<"isClangNoInline", [CXX11<"clang", "noinline">,
-                                                C23<"clang", "noinline">]>];
+                                                C23<"clang", "noinline">,
+                                                CXX11<"msvc", "noinline">,
+                                                C23<"msvc", "noinline">]>];
   let Documentation = [NoInlineDocs];
   let Subjects = SubjectList<[Function, Stmt], WarnDiag,
                              "functions and statements">;
diff --git a/clang/test/CodeGen/attr-ms-noinline.cpp b/clang/test/CodeGen/attr-ms-noinline.cpp
new file mode 100644
index 0000000000000..99b0a05af715d
--- /dev/null
+++ b/clang/test/CodeGen/attr-ms-noinline.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+
+bool bar();
+void f(bool, bool);
+void g(bool);
+
+static int baz(int x) {
+    return x * 10;
+}
+
+[[msvc::noinline]] bool noi() { }
+
+void foo(int i) {
+  [[msvc::noinline]] bar();
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR:[0-9]+]]
+  [[msvc::noinline]] i = baz(i);
+// CHECK: call noundef i32 @_ZL3bazi({{.*}}) #[[NOINLINEATTR]]
+  [[msvc::noinline]] (i = 4, bar());
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+  [[msvc::noinline]] (void)(bar());
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+  [[msvc::noinline]] f(bar(), bar());
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+// CHECK: call void @_Z1fbb({{.*}}) #[[NOINLINEATTR]]
+  [[msvc::noinline]] [] { bar(); bar(); }(); // noinline only applies to the anonymous function call
+// CHECK: call void @"_ZZ3fooiENK3$_0clEv"(ptr {{[^,]*}} %ref.tmp) #[[NOINLINEATTR]]
+  [[msvc::noinline]] for (bar(); bar(); bar()) {}
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+  bar();
+// CHECK: call noundef zeroext i1 @_Z3barv()
+  [[msvc::noinline]] noi();
+// CHECK: call noundef zeroext i1 @_Z3noiv()
+  noi();
+// CHECK: call noundef zeroext i1 @_Z3noiv()
+}
+
+struct S {
+  friend bool operator==(const S &LHS, const S &RHS);
+};
+
+void func(const S &s1, const S &s2) {
+  [[msvc::noinline]]g(s1 == s2);
+// CHECK: call noundef zeroext i1 @_ZeqRK1SS1_({{.*}}) #[[NOINLINEATTR]]
+// CHECK: call void @_Z1gb({{.*}}) #[[NOINLINEATTR]]
+  bool b;
+  [[msvc::noinline]] b = s1 == s2;
+// CHECK: call noundef zeroext i1 @_ZeqRK1SS1_({{.*}}) #[[NOINLINEATTR]]
+}
+
+// CHECK: attributes #[[NOINLINEATTR]] = { noinline }
diff --git a/clang/test/Sema/attr-ms-noinline.cpp b/clang/test/Sema/attr-ms-noinline.cpp
new file mode 100644
index 0000000000000..292d34a41e411
--- /dev/null
+++ b/clang/test/Sema/attr-ms-noinline.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s -Wno-c++17-extensions
+
+int bar();
+
+void foo() {
+  [[msvc::noinline]] bar();
+  [[msvc::noinline(0)]] bar(); // expected-error {{'noinline' attribute takes no arguments}}
+  int x;
+  [[msvc::noinline]] x = 0; // expected-warning {{'noinline' attribute is ignored because there exists no call expression inside the statement}}
+  [[msvc::noinline]] { asm("nop"); } // expected-warning {{'noinline' attribute is ignored because there exists no call expression inside the statement}}
+  [[msvc::noinline]] label: x = 1; // expected-warning {{'noinline' attribute only applies to functions and statements}}
+
+
+  [[msvc::noinline]] always_inline_fn(); // expected-warning {{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+  [[msvc::noinline]] flatten_fn(); // expected-warning {{statement attribute 'noinline' has higher precedence than function attribute 'flatten'}}
+  [[msvc::noinline]] noinline_fn();
+}
+
+[[msvc::noinline]] static int i = bar(); // expected-warning {{'noinline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template<int D>
+int foo(int x) {
+  [[msvc::noinline]] return foo<D-1>(x + 1);
+}
+
+template<int D>
+[[msvc::always_inline]]
+int dependent(int x){ return x + D;} // #DEP
+[[msvc::always_inline]]
+int non_dependent(int x){return x;} // #NO_DEP
+
+template<int D> [[clang::always_inline]]
+int baz(int x) { // #BAZ
+  // expected-warning@+2{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+  // expected-note@#NO_DEP{{conflicting attribute is here}}
+  [[msvc::noinline]] non_dependent(x);
+  if constexpr (D>0) {
+    // expected-warning@+6{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+    // expected-note@#NO_DEP{{conflicting attribute is here}}
+    // expected-warning@+4 3{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+    // expected-note@#BAZ 3{{conflicting attribute is here}}
+    // expected-note@#BAZ_INST 3{{in instantiation}}
+    // expected-note@+1 3{{in instantiation}}
+    [[msvc::noinline]] return non_dependent(x), baz<D-1>(x + 1);
+  }
+  return x;
+}
+
+// We can't suppress if there is a variadic involved.
+template<int ... D>
+int variadic_baz(int x) {
+  // Diagnoses NO_DEP 2x, once during phase 1, the second during instantiation.
+  // Dianoses DEP 3x, once per variadic expansion.
+  // expected-warning@+5 2{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+  // expected-note@#NO_DEP 2{{conflicting attribute is here}}
+  // expected-warning@+3 3{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+  // expected-note@#DEP 3{{conflicting attribute is here}}
+  // expected-note@#VARIADIC_INST{{in instantiation}}
+  [[msvc::noinline]] return non_dependent(x) + (dependent<D>(x) + ...);
+}
+
+void use() {
+  baz<3>(0); // #BAZ_INST
+  variadic_baz<0, 1, 2>(0); // #VARIADIC_INST
+}

simonzgx · 2024-05-13T08:51:30Z

Hi @cor3ntin, could you please help review this patch?

simonzgx · 2024-05-17T14:37:25Z

Hi @cor3ntin , this is just a gentle reminder to confirm if it is still convenient for you to review this patch, or if you could assist in inviting other appropriate reviewers? Thank you.

erichkeane · 2024-05-17T14:44:56Z

clang/include/clang/Basic/Attr.td

@@ -1997,9 +1997,12 @@ def Convergent : InheritableAttr {
 def NoInline : DeclOrStmtAttr {
  let Spellings = [CustomKeyword<"__noinline__">, GCC<"noinline">,
                   CXX11<"clang", "noinline">, C23<"clang", "noinline">,
+                   CXX11<"msvc", "noinline">, C23<"msvc", "noinline">,
                   Declspec<"noinline">];
  let Accessors = [Accessor<"isClangNoInline", [CXX11<"clang", "noinline">,


We should probably have a different name for this, since it is no longer accurate. Perhaps isStmtNoInline

Do you mean to add a new accessors and name it isStmtNoInline?

I think just a rename. This is only used to reject this on a statement if it is NOT (currently) the clang spelling, then suggest the clang spelling. I think the behavior of suggesting [[clang::noinline]] is still fine, but isClangNoInline isn't an accurate name if spelled [[msvc::noinline]]

okay, got it.

erichkeane · 2024-05-17T14:47:15Z

clang/test/Sema/attr-ms-noinline.cpp

@@ -0,0 +1,72 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s -Wno-c++17-extensions


This test is a copy/paste of the other one, I'd rather see this added into the other test/merged instead to show off both spellings in one place (particularly because that is attr-noinline.cpp).

Okay, will fix.

erichkeane · 2024-05-17T14:47:38Z

clang/test/CodeGen/attr-ms-noinline.cpp

@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s


Perhaps the same here, re merging the tests.

erichkeane

You seem to have lost the tests in the latest version of this commit.

simonzgx · 2024-05-21T11:42:08Z

You seem to have lost the tests in the latest version of this commit.

Have already added.

simonzgx · 2024-05-27T03:38:51Z

Hi @erichkeane , can this patch be merged? Or if there is anything else I need to do, please let me know.

Fixes llvm#90941. Add support for ``[[msvc::noinline]]`` attribute, which is actually an alias of ``[[clang::noinline]]``.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 10, 2024

simonzgx force-pushed the support_msvc_noinline_attr branch from 1d7973b to e17ba55 Compare May 10, 2024 09:46

simonzgx marked this pull request as draft May 10, 2024 09:47

simonzgx force-pushed the support_msvc_noinline_attr branch from e17ba55 to 2029279 Compare May 11, 2024 00:51

simonzgx mentioned this pull request May 11, 2024

clang does not support / recognize attribute [[msvc::noinline]] #90941

Closed

simonzgx marked this pull request as ready for review May 11, 2024 08:46

erichkeane reviewed May 17, 2024

View reviewed changes

simonzgx added 3 commits May 19, 2024 16:47

[Clang] Add support for [[msvc::noinline]] attribute. (llvm#90941)

54b6971

add unit test

bbc27cb

fix unit test compile error

4aea776

simonzgx force-pushed the support_msvc_noinline_attr branch from 3f834a5 to 4aea776 Compare May 19, 2024 08:47

update accessor name & delete redundant test

c382cbd

simonzgx force-pushed the support_msvc_noinline_attr branch from 7315477 to c382cbd Compare May 19, 2024 11:58

erichkeane reviewed May 20, 2024

View reviewed changes

add test cases for msvc::noinline

99465de

erichkeane approved these changes May 21, 2024

View reviewed changes

erichkeane merged commit 8995ccc into llvm:main May 28, 2024
4 checks passed

vg0204 pushed a commit to vg0204/llvm-project that referenced this pull request May 29, 2024

[Clang] Add support for [[msvc::noinline]] attribute. (llvm#91720)

4a4c449

Fixes llvm#90941. Add support for ``[[msvc::noinline]]`` attribute, which is actually an alias of ``[[clang::noinline]]``.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Add support for [[msvc::noinline]] attribute. #91720

[Clang] Add support for [[msvc::noinline]] attribute. #91720

simonzgx commented May 10, 2024 •

edited

Loading

llvmbot commented May 10, 2024

simonzgx commented May 13, 2024

simonzgx commented May 17, 2024

erichkeane May 17, 2024

simonzgx May 17, 2024

erichkeane May 17, 2024

simonzgx May 17, 2024

simonzgx May 19, 2024

erichkeane May 17, 2024

simonzgx May 17, 2024

erichkeane May 17, 2024

erichkeane left a comment

simonzgx commented May 21, 2024

simonzgx commented May 27, 2024

		@@ -0,0 +1,72 @@
		// RUN: %clang_cc1 -verify -fsyntax-only %s -Wno-c++17-extensions

		@@ -0,0 +1,53 @@
		// RUN: %clang_cc1 -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - \| FileCheck %s

[Clang] Add support for [[msvc::noinline]] attribute. #91720

[Clang] Add support for [[msvc::noinline]] attribute. #91720

Conversation

simonzgx commented May 10, 2024 • edited Loading

llvmbot commented May 10, 2024

simonzgx commented May 13, 2024

simonzgx commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

simonzgx commented May 21, 2024

simonzgx commented May 27, 2024

simonzgx commented May 10, 2024 •

edited

Loading