This is an archive of the discontinued LLVM Phabricator instance.

[Clang] noinline call site attribute
ClosedPublic

Authored by xbolva00 on Feb 5 2022, 11:05 AM.

Details

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.

Diff Detail

Event Timeline

xbolva00 requested review of this revision.Feb 5 2022, 11:05 AM
xbolva00 created this revision.

Missing in this patch:

  • Some docs
  • Release notes
xbolva00 edited the summary of this revision. (Show Details)Feb 5 2022, 11:07 AM
xbolva00 edited the summary of this revision. (Show Details)
xbolva00 added a reviewer: rsmith.
xbolva00 updated this revision to Diff 406198.Feb 5 2022, 11:50 AM
xbolva00 added a subscriber: RKSimon.

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 added inline comments.Feb 10 2022, 5:17 AM
clang/include/clang/Basic/Attr.td
1763–1770

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
208–210
224
clang/test/CodeGen/attr-noinline.cpp
16–17

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
12
15

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

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

Thank you for initial review

clang/include/clang/Basic/Attr.td
1763–1770

Agree, will do.

1764

Copy pasting for nomerge :(

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

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

Then we have some issue somewhere deeper :)

15

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 added inline comments.Feb 10 2022, 6:15 AM
clang/test/Sema/attr-noinline.cpp
12

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.

15

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 added inline comments.
clang/test/Sema/attr-noinline.cpp
15

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 added inline comments.Feb 10 2022, 9:44 AM
clang/test/Sema/attr-noinline.cpp
15

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

xbolva00 planned changes to this revision.Feb 10 2022, 1:51 PM

Plan:

  1. https://reviews.llvm.org/D119451
  2. fix inliner in LLVM
  3. continue with this patch
kuhar added a comment.EditedFeb 10 2022, 1:59 PM

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?
  1. 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.

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 added a comment.Feb 10 2022, 2:16 PM

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 added a comment.EditedFeb 10 2022, 2:31 PM

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 = ]

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

https://godbolt.org/z/jr3Wx87rb

Fixes for inliners landed, working on this now.

xbolva00 updated this revision to Diff 408573.Feb 14 2022, 12:44 PM

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

kuhar added inline comments.Feb 14 2022, 12:47 PM
clang/test/CodeGen/attr-noinline.cpp
12

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

xbolva00 added inline comments.Feb 14 2022, 12:47 PM
clang/include/clang/Basic/AttrDocs.td
533

Feel free to suggest better wording :)

clang/include/clang/Basic/DiagnosticSemaKinds.td
4042

Feel free to suggest better wording :)

4043

Ideally this would be something like

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

but...

clang/lib/Sema/SemaStmtAttr.cpp
232

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

clang/test/CodeGen/attr-noinline.cpp
16–17

Added as tests. Yes, not inlined.

xbolva00 updated this revision to Diff 408578.Feb 14 2022, 12:50 PM
xbolva00 updated this revision to Diff 408580.Feb 14 2022, 12:53 PM
xbolva00 updated this revision to Diff 408588.Feb 14 2022, 1:08 PM

More tests

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

Yes, added

xbolva00 added inline comments.Feb 14 2022, 1:12 PM
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?

kuhar accepted this revision.EditedFeb 23 2022, 10:24 AM

LGTM but please get at least one additional approval before submitting

This revision is now accepted and ready to land.Feb 23 2022, 10:24 AM

LGTM but please get at least one additional approval before submitting

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

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 updated this revision to Diff 411383.Feb 25 2022, 5:15 AM

Fixed tests.

Addressed review feedback.

xbolva00 added inline comments.Feb 25 2022, 5:15 AM
clang/include/clang/Basic/Attr.td
1764

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

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

clang/include/clang/Basic/AttrDocs.td
536

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 added inline comments.Feb 25 2022, 10:47 AM
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 added inline comments.Feb 25 2022, 10:51 AM
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 added inline comments.Feb 25 2022, 11:06 AM
clang/test/Sema/attr-noinline.cpp
24

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

xbolva00 updated this revision to Diff 411489.Feb 25 2022, 12:03 PM

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

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

xbolva00 updated this revision to Diff 411495.Feb 25 2022, 12:25 PM

Small doc update.

xbolva00 updated this revision to Diff 411589.Feb 26 2022, 1:56 AM

Fixed failing Parser test.

xbolva00 edited the summary of this revision. (Show Details)Feb 28 2022, 12:19 PM
This revision was landed with ongoing or failed builds.Feb 28 2022, 12:21 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2022, 12:21 PM
dewen added a subscriber: dewen.May 3 2023, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 11:33 PM
Via Web