[Clang] noinline call site attribute

[xbolva00]

Summary

Motivation:

int foo(int x, int y) { // any compiler will happily inline this function
    return x / y;
}

int test(int x, int y) {
    int r = 0;
    [[clang::noinline]] r += foo(x, y); // for some reason we don't want any inlining here
    return r;
}

In 2018, @kuhar proposed "Introduce per-callsite inline intrinsics" in https://reviews.llvm.org/D51200 to solve this motivation case (and many others).

This patch solves this problem with call site attribute. The implementation is "smaller" wrt approach which uses new intrinsics and thanks to https://reviews.llvm.org/D79121 (Add nomerge statement attribute to clang), we have got some basic infrastructure to deal with attrs on statements with call expressions.

GCC devs are more inclined to call attribute solution as well, as builtins are problematic for them - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104187. But they have no patch proposal yet so.. We have free hands here.

If this approach makes sense, next future steps would be support for call site attributes for always_inline / flatten.

[xbolva00]

Missing in this patch:

• Some docs
• Release notes

[xbolva00]

ping

[aaron.ballman]

Precommit CI failures look related to the changes here, so you should investigate those.

clang/include/clang/Basic/Attr.td

1764

Can you explain why you made this change? This seems like it'll do the wrong thing for functions because it'll now be duplicating the attribute on each redeclaration. e.g.,

[[clang::noinline]] void func(); // Has NoInlineAttr x1
[[clang::noinline]] void func(); // Has NoInlineAttr x2
void func(); // Has NoInlineAttr x2
[[clang::noinline]] void func() { // Has NoInlineAttr x3
}

(It seems we already have this bug with nomerge: https://godbolt.org/z/aMTcTdPEP)

[aaron.ballman]

clang/include/clang/Basic/Attr.td

1763

It seems like it's time to write some documentation for this attribute now that we're modifying it significantly.

clang/lib/Sema/SemaStmtAttr.cpp

204–205

219

clang/test/CodeGen/attr-noinline.cpp

15–16

I'd also like to see the behavior tested with blocks and statement expressions.

Also, I'd like to see test cases where the call is "hidden" from the user's sight. e.g.,

struct S {
  friend bool operator==(const S &LHS, const S &RHS);
};

void f(bool);

void func(const S &s1, const S &s2) {
  [[clang::noinline]]f(s1 == s2); // Is operator==() then not inlined?
}

clang/test/Sema/attr-noinline.cpp

11

Huh? this is a statement: http://eel.is/c++draft/stmt.label#nt:labeled-statement

14

Should there be any diagnostic in a situation like this?

[[gnu::always_inline]] int f() { return 0; }

void func() {
  [[clang::noinline]] int i = f(); // Should there be some way to know of the conflict?
}

[aaron.ballman]

Oh, also, this seems worth adding a release note over. :-)

[xbolva00]

Thank you for initial review

clang/include/clang/Basic/Attr.td

1763

Agree, will do.

1764

Copy pasting for nomerge :(

I will send a patch to fix it for nomerge :)

clang/test/Sema/attr-noinline.cpp

11

Then we have some issue somewhere deeper :)

14

Yeah, this is tricky. LLVM will currently prefer always_inline.

I was wondering about a new warning as well, I think it is a good idea to warn tor this scenario, I was just not sure where is a good place for such check. Maybe collect all callexprs in CallExprFinder and detech this colision?

[aaron.ballman]

clang/test/Sema/attr-noinline.cpp

11

Oh, I think I know what's going on there: we're trying to apply the attribute to the LabelDecl and not to the LabelStmt. I don't think you need to solve it for this patch.

14

Yeah, if always_inline wins, that'd be a pretty big surprise. Is that intentional, or expected to change? (I would have expected the call site attribute to "win" because it knows more about its inlining needs than the author of the declaration.)

As for the diagnostic, the call expression finder is where I'd expect the logic to start from. That'll find you all the call expressions involved, and for each one of those you can look to see if you can find a decl for the call, and if there is one, check to see if it has an AlwaysInlineAttr on it, then diagnose from there.

[xbolva00]

clang/test/Sema/attr-noinline.cpp

14

Yes, I think that noinline should win as well :) but it is kinda edge case scenario, so I am not too worried for now.

But yeah, fix for inliner probably makes sense.

WDYT @aeubanks ? Would be such small change acceptable? I could send a patch.

[aeubanks]

clang/test/Sema/attr-noinline.cpp

14

favoring call-site attributes over callee attributes in the inliner seems reasonable

[xbolva00]

Plan:

1. https://reviews.llvm.org/D119451
2. fix inliner in LLVM
3. continue with this patch

[kuhar]

Thanks for working on this, @xbolva00! I don't know this part of the codebase, so won't comment on the patch itself. Just a few questions/suggestions:

1. Do you plan to also add inline and flatten?

2. When I worked on my implementation, I remember that I also ran into the issue of conflicting attributes. I defaulted to whatever the inliner behavior was at the time, but a few folks pointed out to me that this can be very confusing. I think this needs thorough documentation / testing.

[xbolva00]

Do you plan to also add inline and flatten?

You mean always_inline? Yes, after noinline. The flatten call site attribute - theoretically why not, but it needs to be reworked in LLVM (like always_inline_recursively) before any patch like this one.

When I worked on my implementation, I remember that I also ran into the issue of conflicting attributes. I defaulted to whatever the inliner behavior was at the time, but a few folks pointed out to me that this can be very confusing. I think this needs thorough documentation / testing.

Yes, as we mentioned it already, for example always_inline on decl, and noinline on callsite. We should diagnose these cases and always prefer call site attribute. (and as I said, some fixes for inliner are needed).

[kuhar]

In D119061#3312571, @xbolva00 wrote:

Do you plan to also add inline and flatten?

You mean always_inline? Yes, after noinline. The flatten call site attribute - theoretically why not, but it needs to be reworked in LLVM (like always_inline_recursively) before any patch like this one.

SGTM.

When I worked on my implementation, I remember that I also ran into the issue of conflicting attributes. I defaulted to whatever the inliner behavior was at the time, but a few folks pointed out to me that this can be very confusing. I think this needs thorough documentation / testing.

Yes, as we mentioned it already, for example always_inline on decl, and noinline on callsite. We should diagnose these cases and always prefer call site attribute. (and as I said, some fixes for inliner are needed).

It would be good to check with a language expert if this can break some code. I'm thinking of situations when someone relies on a function being emitted (or not) and ending up with linking issues. I'm not an expert here, but this is a past concern I vaguely remember.

[xbolva00]

It would be good to check with a language expert if this can break some code. I'm thinking of situations when someone relies on a function being emitted (or not) and ending up with linking issues. I'm not an expert here, but this is a past concern I vaguely remember.

currently flatten attribute is implemented in way that always_inline attribute is attached to all calls, so inliners already know about call site attributes.

if this can break some code

This is totally opt-in, I am not sure how this can break any current codebases, as noinline attribute on statements was strongly rejected.

I'm thinking of situations when someone relies on a function being emitted (or not) and ending up with linking issues.

Hm, I am thinking about our common edge case:

always_inline foo() { }

bar () {

noinline foo();

}

Need to check inliner if foo stays.

Maybe just strongly reject conflicting attributes and declare win here? Now I think it could be better idea to emit an error in this case instead of thinking about all edge cases.
also..

static int foo(int x) {
    return x * 10;
}


int main(void) {
    int x;
    /* noinline attr */ x = foo(4);
    return x;
}

this is also interesting testcase to reject = ]

[xbolva00]

Interestly, clang/llvm happily ignores noinline on static functions, gcc does not.

https://godbolt.org/z/jr3Wx87rb

[xbolva00]

Fixes for inliners landed, working on this now.

[xbolva00]

Added check to warn for conflicting attributes.
More docs, tests.

[kuhar]

clang/test/CodeGen/attr-noinline.cpp

12

Could you also add a test function that is already marked as noinline at the declaration?

[xbolva00]

clang/include/clang/Basic/AttrDocs.td

522 ↗

(On Diff #408573)

Feel free to suggest better wording :)

clang/include/clang/Basic/DiagnosticSemaKinds.td

4038

Feel free to suggest better wording :)

4039

Ideally this would be something like

"statement attribute %0 has higher precedence than function attribute %1"

but...

clang/lib/Sema/SemaStmtAttr.cpp

227

.. not sure how to get AttributeCommonInfo here so I could use "<< X".

clang/test/CodeGen/attr-noinline.cpp

15–16

Added as tests. Yes, not inlined.

[xbolva00]

More tests

clang/test/CodeGen/attr-noinline.cpp

12

Yes, added

[xbolva00]

clang/include/clang/Basic/Attr.td

1764

What I need to mention here. With Clang<"noinline">, we lost support for [[gnu::noinline]] syntax, now unrecognized. Maybe it is ok, as we have now more powerful attribute?

[xbolva00]

ping :)

[xbolva00]

ping

[kuhar]

LGTM but please get at least one additional approval before submitting

[xbolva00]

In D119061#3340884, @kuhar wrote:

LGTM but please get at least one additional approval before submitting

Thank you. Yeah, I would like to get lgtm from @aaron.ballman here :)

[aaron.ballman]

Precommit CI seems to be failing with relevant failures:

Failed Tests (2):
  Clang :: Parser/stmt-attributes.c
  Clang :: Sema/attr-noinline.c

clang/include/clang/Basic/Attr.td

1764

Nope, we need to retain at least part of GCC<"noinline"> because people may be relying on [[gnu::noinline]]. The GNU version should be disallowed as a statement attribute unless GCC gets the same functionality. I suspect using the GCC spelling may cause issues (because GCC spelling implies a GNU attribute as does the Clang spelling). So you may need to use CXX11<"gnu", "noinline"> and C2x<"gnu", "noinline"> to retain those spellings.

Btw, you can use an Accessors field here to give the semantic attribute the ability to differentiate between the GCC and the Clang spellings. The GNU spelling is a bit of a question mark; I think it's fine to allow it as a statement attribute, but we might want to warn that the use is not compatible with GCC.

You'll have to update the documentation to describe what's supported where and under which spelling.

[xbolva00]

Fixed tests.

Addressed review feedback.

[xbolva00]

clang/include/clang/Basic/Attr.td

1764

Thanks for advices, Accessors is really useful here. Added warning and updated doc a bit.

[aaron.ballman]

Also, it looks like there are some clang-format nits that can be addressed.

clang/include/clang/Basic/AttrDocs.td

525 ↗

(On Diff #411383)

Maybe we should also say "Only the [[clang::noinline]] spelling can be used as a statement attribute; other spellings of the attribute are not supported on statements." or something to that effect. WDYT?

Adding some code examples to clarify would also not go amiss.

clang/test/Sema/attr-noinline.cpp

24

It might be nice to clarify the diagnostic; it's not that noinline attribute is ignored always, it's that [[gnu::noinline]] and __attribute__((noinline)) are ignored specifically.

Can you add one more test case for:

__attribute__((noinline)) bar();

(I would expect this to also diagnose.)

[xbolva00]

clang/test/Sema/attr-noinline.cpp

24

__attribute__((noinline)) bar();

is not diagnosed, as "// The Clang spelling implies GNU<name>, CXX11<"clang", name>, and optionally," :[] Not sure if this could be easily fixed.

[aaron.ballman]

clang/test/Sema/attr-noinline.cpp

24

That's what I suspected -- I think we should diagnose this case.

I think I had my idea backwards in Attr.td. We should use GCC<"noinline"> as the spelling, and add CXX11<"clang", "noinline"> and C2x<"clang", "noinline">; then the GNU spelling is associated with GCC. Then you can change the accessor for the attribute and that should fix this issue.

[xbolva00]

clang/test/Sema/attr-noinline.cpp

24

Ok, it is possible to swap Spellings to handle it as well.

[xbolva00]

Addressed review comments. Feel free to suggest better wording :)

[xbolva00]

I think I will update release notes later, after always_inline variant of this patch. So looks like we are almost done here? :)

[xbolva00]

Small doc update.

[xbolva00]

Fixed failing Parser test.

[aaron.ballman]

LGTM!

clang/include/clang/Basic/Attr.td

 
 def Convergent : InheritableAttr {
   let Spellings = [Clang<"convergent">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [ConvergentDocs];
   let SimpleHandler = 1;
 }
 
-def NoInline : InheritableAttr {
-  let Spellings = [GCC<"noinline">, Declspec<"noinline">];
-  let Subjects = SubjectList<[Function]>;
+def NoInline : DeclOrStmtAttr {
+  let Spellings = [Clang<"noinline">, Declspec<"noinline">];
   let Documentation = [Undocumented];
+  let InheritEvenIfAlreadyPresent = 1;
+  let Subjects = SubjectList<[Function, Stmt], ErrorDiag,
+                             "functions and statements">;
   let SimpleHandler = 1;
 }
 
 def NoMips16 : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"nomips16">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 // Objective-C++
 def err_objc_decls_may_only_appear_in_global_scope : Error<
   "Objective-C declarations may only appear in global scope">;
 def warn_auto_var_is_id : Warning<
   "'auto' deduced as 'id' in declaration of %0">,
   InGroup<DiagGroup<"auto-var-id">>;
 
 // Attributes
-def warn_nomerge_attribute_ignored_in_stmt: Warning<
+def warn_attribute_ignored_in_stmt: Warning<
   "%0 attribute is ignored because there exists no call expression inside the "
   "statement">,
   InGroup<IgnoredAttributes>;
 
 def err_musttail_needs_trivial_args : Error<
   "tail call requires that the return value, all parameters, and any "
   "temporaries created by the expression are trivially destructible">;
 def err_musttail_needs_call : Error<
def err_enum_mode_vector_type : Error<
   "mode %0 is not supported for enumeration types">;
 def warn_attribute_nonnull_no_pointers : Warning<
   "'nonnull' attribute applied to function with no pointer arguments">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_nonnull_parm_no_args : Warning<
   "'nonnull' attribute when used on parameters takes no arguments">,
   InGroup<IgnoredAttributes>;
 def note_declared_nonnull : Note<
   "declared %select{'returns_nonnull'|'nonnull'}0 here">;
 def warn_attribute_sentinel_named_arguments : Warning<
   "'sentinel' attribute requires named arguments">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_sentinel_not_variadic : Warning<
   "'sentinel' attribute only supported for variadic %select{functions|blocks}0">,
   InGroup<IgnoredAttributes>;
 def warn_deprecated_ignored_on_using : Warning<
   "%0 currently has no effect on a using declaration">,
   InGroup<IgnoredAttributes>;

clang/lib/CodeGen/CGCall.cpp

if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl))
     if (FD->hasAttr<StrictFPAttr>())
       // All calls within a strictfp function are marked strictfp
       Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::StrictFP);
 
   // Add call-site nomerge attribute if exists.
   if (InNoMergeAttributedStmt)
     Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoMerge);
 
+  // Add call-site noinline attribute if exists.
+  if (InNoInlineAttributedStmt)
+    Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);
+
   // Apply some call-site-specific attributes.
   // TODO: work this into building the attribute set.
 
   // Apply always_inline to all calls within flatten functions.
   // FIXME: should this really take priority over __try, below?
   if (CurCodeDecl && CurCodeDecl->hasAttr<FlattenAttr>() &&
+      !InNoInlineAttributedStmt &&
       !(TargetDecl && TargetDecl->hasAttr<NoInlineAttr>())) {
     Attrs =
         Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::AlwaysInline);
   }
 
   // Disable inlining inside SEH __try blocks.
   if (isSEHTryScope()) {
     Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);

clang/lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitLabelStmt(const LabelStmt &S) {
   if (getLangOpts().EHAsynch && S.isSideEntry())
     EmitSehCppScopeBegin();
 
   EmitStmt(S.getSubStmt());
 }
 
 void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
   bool nomerge = false;
+  bool noinline = false;
   const CallExpr *musttail = nullptr;
 
   for (const auto *A : S.getAttrs()) {
     if (A->getKind() == attr::NoMerge) {
       nomerge = true;
     }
+    if (A->getKind() == attr::NoInline) {
+      noinline = true;
+    }
     if (A->getKind() == attr::MustTail) {
       const Stmt *Sub = S.getSubStmt();
       const ReturnStmt *R = cast<ReturnStmt>(Sub);
       musttail = cast<CallExpr>(R->getRetValue()->IgnoreParens());
     }
   }
   SaveAndRestore<bool> save_nomerge(InNoMergeAttributedStmt, nomerge);
+  SaveAndRestore<bool> save_noinline(InNoInlineAttributedStmt, noinline);
   SaveAndRestore<const CallExpr *> save_musttail(MustTailCall, musttail);
   EmitStmt(S.getSubStmt(), S.getAttrs());
 }
 
 void CodeGenFunction::EmitGotoStmt(const GotoStmt &S) {
   // If this code is reachable then emit a stop point (if generating
   // debug info). We have to do this ourselves because we are on the
   // "simple" statement path.

clang/lib/CodeGen/CodeGenFunction.h

public:
 
   /// True if CodeGen currently emits code inside presereved access index
   /// region.
   bool IsInPreservedAIRegion = false;
 
   /// True if the current statement has nomerge attribute.
   bool InNoMergeAttributedStmt = false;
 
+  /// True if the current statement has noinline attribute.
+  bool InNoInlineAttributedStmt = false;
+
   // The CallExpr within the current statement that the musttail attribute
   // applies to.  nullptr if there is no 'musttail' on the current statement.
   const CallExpr *MustTailCall = nullptr;
 
   /// Returns true if a function must make progress, which means the
   /// mustprogress attribute can be added.
   bool checkIfFunctionMustProgress() {
     if (CGM.getCodeGenOpts().getFiniteLoops() ==

clang/lib/Sema/SemaStmtAttr.cpp

 } // namespace
 
 static Attr *handleNoMergeAttr(Sema &S, Stmt *St, const ParsedAttr &A,
                                SourceRange Range) {
   NoMergeAttr NMA(S.Context, A);
   CallExprFinder CEF(S, St);
 
   if (!CEF.foundCallExpr()) {
-    S.Diag(St->getBeginLoc(), diag::warn_nomerge_attribute_ignored_in_stmt)
+    S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_in_stmt)
         << NMA.getSpelling();
     return nullptr;
   }
 
   return ::new (S.Context) NoMergeAttr(S.Context, A);
 }
 
+static Attr *handleNoInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
+                                SourceRange Range) {
+  NoInlineAttr NIA(S.Context, A);
+  CallExprFinder CEF(S, St);
+
+  if (!CEF.foundCallExpr()) {
+    S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_in_stmt)
+        << NIA.getSpelling();
+    return nullptr;
+  }
+
+  return ::new (S.Context) NoInlineAttr(S.Context, A);
+}
+
 static Attr *handleMustTailAttr(Sema &S, Stmt *St, const ParsedAttr &A,
                                 SourceRange Range) {
   // Validation is in Sema::ActOnAttributedStmt().
   return ::new (S.Context) MustTailAttr(S.Context, A);
 }
 
 static Attr *handleLikely(Sema &S, Stmt *St, const ParsedAttr &A,
                           SourceRange Range) {
 
   if (!S.getLangOpts().CPlusPlus20 && A.isCXX11Attribute() && !A.getScopeName())
static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
   case ParsedAttr::AT_LoopHint:
     return handleLoopHintAttr(S, St, A, Range);
   case ParsedAttr::AT_OpenCLUnrollHint:
     return handleOpenCLUnrollHint(S, St, A, Range);
   case ParsedAttr::AT_Suppress:
     return handleSuppressAttr(S, St, A, Range);
   case ParsedAttr::AT_NoMerge:
     return handleNoMergeAttr(S, St, A, Range);
+  case ParsedAttr::AT_NoInline:
+    return handleNoInlineAttr(S, St, A, Range);
   case ParsedAttr::AT_MustTail:
     return handleMustTailAttr(S, St, A, Range);
   case ParsedAttr::AT_Likely:
     return handleLikely(S, St, A, Range);
   case ParsedAttr::AT_Unlikely:
     return handleUnlikely(S, St, A, Range);
   default:
     // N.B., ClangAttrEmitter.cpp emits a diagnostic helper that ensures a

clang/test/CodeGen/attr-noinline.cpp

+// RUN: %clang_cc1 -S -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+
+bool bar();
+void f(bool, bool);
+
+void foo(int i) {
+  [[clang::noinline]] bar();
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR:[0-9]+]]
+  [[clang::noinline]] (i = 4, bar());
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+  [[clang::noinline]] (void)(bar());
+// CHECK: call noundef zeroext i1 @_Z3barv() #[[NOINLINEATTR]]
+  [[clang::noinline]] f(bar(), bar());
+// CHECK: call void @_Z1fbb({{.*}}) #[[NOINLINEATTR]]
+  [[clang::noinline]] [] { bar(); bar(); }(); // noinline only applies to the anonymous function call
+// CHECK: call void @"_ZZ3fooiENK3$_0clEv"(%class.anon* {{[^,]*}} %ref.tmp) #[[NOINLINEATTR]]
+  [[clang::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()
+}
+
+// CHECK: attributes #[[NOINLINEATTR]] = { noinline }

clang/test/Sema/attr-noinline.cpp

+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+int bar();
+
+void foo() {
+  [[clang::noinline]] bar();
+  [[clang::noinline(0)]] bar(); // expected-error {{'noinline' attribute takes no arguments}}
+  int x;
+  [[clang::noinline]] x = 0; // expected-warning {{noinline attribute is ignored because there exists no call expression inside the statement}}
+
+  [[clang::noinline]] label: x = 1; // expected-error {{'noinline' attribute only applies to functions and statements}}
+
+}
+
+[[clang::noinline]] static int i = bar(); // expected-error {{'noinline' attribute only applies to functions and statements}}