[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a subscriber: probinson.
jdoerfert added a comment.

Found the commit: dcbe35bad518

The way I see it we just have to teach the inliner about `optnone` so we can 
uncouple the two (`optnone` and `noinline`).
@probinson WDTY?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D70366#1972880 , @LevitatingLion 
wrote:

> In D70366#1971137 , @dexonsmith 
> wrote:
>
> > In D70366#1970758 , @jdoerfert 
> > wrote:
> >
> > > In D70366#1970526 , @dexonsmith 
> > > wrote:
> > >
> > > > In D70366#1970299 , 
> > > > @LevitatingLion wrote:
> > > >
> > > > > Maybe we can add an additional string attribute when adding the 
> > > > > noinline attribute to functions which are not marked noinline in the 
> > > > > source code, something like "noinline-added-by-clang". I don't know 
> > > > > if that's a legitimate use case for a string attribute, but it 
> > > > > wouldn't be very invasive. What do you think?
> > > >
> > > >
> > > > Another option (not sure if it's better) would be to add a `noopt` LLVM 
> > > > attribute that Clang adds for `-O0` instead of `noinline`.  Two 
> > > > possibilities would be to update the inliner to pay attention to that 
> > > > as well (with special logic for `flatten`), or to change the 
> > > > always-inliner to add `noinline` to anything marked `noopt`.
> > >
> > >
> > > `noopt == optnone`? Both `optnone` and `noinline` are set in O0, so we 
> > > could just not place `noinline` (I think).
> >
> >
> > Sure, that could work.  Or the noflatten idea is another possibility.  It 
> > would be good to hear what others think.
>
>
> `optnone` currently requires `noinline`. Can we simply remove this 
> requirement or would that need more changes?


I don't see a reason why we couldn't.

> If I understand the `noflatten` idea correctly, we would change the LLVM 
> behaviour so that `alwaysinline_recursively` ignores `noinline` and stops 
> inlining only when a callee has a dedicated "stop-marker" attribute (e.g. 
> `noflatten`)? I think that would be counter-intuitive, `noinline` should 
> prevent inlining.

I would prefer the above solution instead of another "stop token".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion added a comment.

In D70366#1971137 , @dexonsmith wrote:

> In D70366#1970758 , @jdoerfert wrote:
>
> > In D70366#1970526 , @dexonsmith 
> > wrote:
> >
> > > In D70366#1970299 , 
> > > @LevitatingLion wrote:
> > >
> > > > Maybe we can add an additional string attribute when adding the 
> > > > noinline attribute to functions which are not marked noinline in the 
> > > > source code, something like "noinline-added-by-clang". I don't know if 
> > > > that's a legitimate use case for a string attribute, but it wouldn't be 
> > > > very invasive. What do you think?
> > >
> > >
> > > Another option (not sure if it's better) would be to add a `noopt` LLVM 
> > > attribute that Clang adds for `-O0` instead of `noinline`.  Two 
> > > possibilities would be to update the inliner to pay attention to that as 
> > > well (with special logic for `flatten`), or to change the always-inliner 
> > > to add `noinline` to anything marked `noopt`.
> >
> >
> > `noopt == optnone`? Both `optnone` and `noinline` are set in O0, so we 
> > could just not place `noinline` (I think).
>
>
> Sure, that could work.  Or the noflatten idea is another possibility.  It 
> would be good to hear what others think.


`optnone` currently requires `noinline`. Can we simply remove this requirement 
or would that need more changes?

If I understand the `noflatten` idea correctly, we would change the LLVM 
behaviour so that `alwaysinline_recursively` ignores `noinline` and stops 
inlining only when a callee has a dedicated "stop-marker" attribute (e.g. 
`noflatten`)? I think that would be counter-intuitive, `noinline` should 
prevent inlining.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D70366#1970758 , @jdoerfert wrote:

> In D70366#1970526 , @dexonsmith 
> wrote:
>
> > In D70366#1970299 , 
> > @LevitatingLion wrote:
> >
> > > Maybe we can add an additional string attribute when adding the noinline 
> > > attribute to functions which are not marked noinline in the source code, 
> > > something like "noinline-added-by-clang". I don't know if that's a 
> > > legitimate use case for a string attribute, but it wouldn't be very 
> > > invasive. What do you think?
> >
> >
> > Another option (not sure if it's better) would be to add a `noopt` LLVM 
> > attribute that Clang adds for `-O0` instead of `noinline`.  Two 
> > possibilities would be to update the inliner to pay attention to that as 
> > well (with special logic for `flatten`), or to change the always-inliner to 
> > add `noinline` to anything marked `noopt`.
>
>
> `noopt == optnone`? Both `optnone` and `noinline` are set in O0, so we could 
> just not place `noinline` (I think).


Sure, that could work.  Or the noflatten idea is another possibility.  It would 
be good to hear what others think.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D70366#1970526 , @dexonsmith wrote:

> In D70366#1970299 , @LevitatingLion 
> wrote:
>
> > Maybe we can add an additional string attribute when adding the noinline 
> > attribute to functions which are not marked noinline in the source code, 
> > something like "noinline-added-by-clang". I don't know if that's a 
> > legitimate use case for a string attribute, but it wouldn't be very 
> > invasive. What do you think?
>
>
> Another option (not sure if it's better) would be to add a `noopt` LLVM 
> attribute that Clang adds for `-O0` instead of `noinline`.  Two possibilities 
> would be to update the inliner to pay attention to that as well (with special 
> logic for `flatten`), or to change the always-inliner to add `noinline` to 
> anything marked `noopt`.


`noopt == optnone`? Both `optnone` and `noinline` are set in O0, so we could 
just not place `noinline` (I think).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D70366#1970526 , @dexonsmith wrote:

> In D70366#1970299 , @LevitatingLion 
> wrote:
>
> > Maybe we can add an additional string attribute when adding the noinline 
> > attribute to functions which are not marked noinline in the source code, 
> > something like "noinline-added-by-clang". I don't know if that's a 
> > legitimate use case for a string attribute, but it wouldn't be very 
> > invasive. What do you think?
>
>
> Another option (not sure if it's better) would be to add a `noopt` LLVM 
> attribute that Clang adds for `-O0` instead of `noinline`.  Two possibilities 
> would be to update the inliner to pay attention to that as well (with special 
> logic for `flatten`), or to change the always-inliner to add `noinline` to 
> anything marked `noopt`.


Or, have clang add a new `noflatten` attribute when it sees 
`__attribute__((noinline))`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D70366#1970299 , @LevitatingLion 
wrote:

> Maybe we can add an additional string attribute when adding the noinline 
> attribute to functions which are not marked noinline in the source code, 
> something like "noinline-added-by-clang". I don't know if that's a legitimate 
> use case for a string attribute, but it wouldn't be very invasive. What do 
> you think?


Another option (not sure if it's better) would be to add a `noopt` LLVM 
attribute that Clang adds for `-O0` instead of `noinline`.  Two possibilities 
would be to update the inliner to pay attention to that as well (with special 
logic for `flatten`), or to change the always-inliner to add `noinline` to 
anything marked `noopt`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D70366#1970375 , @jdoerfert wrote:

> TBH, I would issue a warning if we see `flatten` in O0 that says this will 
> not work and be done with it.


I would argue against diagnostics that depend on optimization level, since that 
leads to an inconsistent developer experience.  In practice developers tend to 
use Debug and Release configurations fairly interchangeably, with different 
optimization levels for each.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D70366#1970299 , @LevitatingLion 
wrote:

> While adding tests to clang I realized the attribute is not working as 
> intended when using an optimization level of zero, because clang adds the 
> noinline attribute to all functions. In this case the optimizer cannot 
> distinguish between functions originally marked noinline (where recursive 
> always-inlining should stop) and those where clang added the attribute (where 
> recursive always-inlining should continue).
>
> Is this acceptable? I think we should fix this, and recursively inline at 
> optimization level zero. GCC's documentation on the flatten attribute states 
> that "every call inside this function is inlined, if possible", clang's that 
> calls are "inlined unless it is impossible to do so".
>
> Maybe we can add an additional string attribute when adding the noinline 
> attribute to functions which are not marked noinline in the source code, 
> something like "noinline-added-by-clang". I don't know if that's a legitimate 
> use case for a string attribute, but it wouldn't be very invasive. What do 
> you think?


TBH, I would issue a warning if we see `flatten` in O0 that says this will not 
work and be done with it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion added a comment.

While adding tests to clang I realized the attribute is not working as intended 
when using an optimization level of zero, because clang adds the noinline 
attribute to all functions. In this case the optimizer cannot distinguish 
between functions originally marked noinline (where recursive always-inlining 
should stop) and those where clang added the attribute (where recursive 
always-inlining should continue).

Is this acceptable? I think we should fix this, and recursively inline at 
optimization level zero. GCC's documentation on the flatten attribute states 
that "every call inside this function is inlined, if possible", clang's that 
calls are "inlined unless it is impossible to do so".

Maybe we can add an additional string attribute when adding the noinline 
attribute to functions which are not marked noinline in the source code, 
something like "noinline-added-by-clang". I don't know if that's a legitimate 
use case for a string attribute, but it wouldn't be very invasive. What do you 
think?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

The semantics of this llvm attribute seem to better match 'flatten'.  However, 
it is unfortunate that this doesn't change any clang tests.  Can you add/alter 
a test in clang to validate the IR?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D70366#1960300 , @jdoerfert wrote:

> I'm fine with this. I would hope a C/C++/Clang person will also take a look 
> though.


This is missing clang codegen test[s].
Seems to look fine to me otherwise.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I'm fine with this. I would hope a C/C++/Clang person will also take a look 
though.




Comment at: llvm/docs/LangRef.rst:1398
+This attribute is similar to ``alwaysinline``, but also applies 
recursively to
+all inlined function calls.
 ``builtin``

Maybe mention the correspondence to the `flatten` C/C++ attribute here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion updated this revision to Diff 254217.
LevitatingLion added a comment.
Herald added a reviewer: sstefan1.

I rebased my changes onto 49d00824bbb 
, renamed 
the attribute to 'alwaysinline_recursively', and added some more tests. The 
testcase 'highLevelStructure.3.2.ll' does not fail anymore, all regression 
tests pass.

Are there any more places where changes are required? I looked at the changes 
when other attributes were introduced and grep'd for 'Attribute::AlwaysInline' 
to find places which need handling of the new attribute.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366

Files:
  clang/lib/CodeGen/CGCall.cpp
  llvm/docs/BitCodeFormat.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/SafeStack.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/AMDGPU/AMDGPUInline.cpp
  llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp
  llvm/lib/Transforms/IPO/AlwaysInliner.cpp
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/lib/Transforms/IPO/HotColdSplitting.cpp
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/lib/Transforms/IPO/PartialInlining.cpp
  llvm/lib/Transforms/IPO/SyntheticCountsPropagation.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/Transforms/Inline/always-inline-recursively.ll

Index: llvm/test/Transforms/Inline/always-inline-recursively.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/always-inline-recursively.ll
@@ -0,0 +1,267 @@
+; RUN: opt < %s -inline-threshold=0 -inline -S | FileCheck %s
+; RUN: opt < %s -inline-threshold=0 -always-inline -S | FileCheck %s
+;
+; Ensure the threshold has no impact on these decisions.
+; RUN: opt < %s -inline-threshold=2000 -inline -S | FileCheck %s
+; RUN: opt < %s -inline-threshold=2000 -always-inline -S | FileCheck %s
+; RUN: opt < %s -inline-threshold=-2000 -inline -S | FileCheck %s
+; RUN: opt < %s -inline-threshold=-2000 -always-inline -S | FileCheck %s
+
+; In the tests involving recursive functions we call the external function and
+; continue recursion depending on the external variable, so that any recursion
+; conditions are opaque to the optimizer
+
+; Following tests are conducted, for annotating call-sites and function declarations:
+;   Test that a simple tree call-graph is inlined
+;   Test that functions marked noinline are not inlined
+;   Test that a recursive call is not inlined
+;   Test that an indirectly recursive call is inlined until a directly recursive call remains
+
+; External funcion not visible to the optimizer
+declare void @ext_func()
+; External variable not visible to the optimizer
+@ext_var = external global i32
+
+; Test that a simple tree call-graph is inlined
+; when annotating call-sites
+
+define void @test_calls_tree() {
+; CHECK-LABEL: @test_calls_tree() {
+  call void @test_calls_tree_1() alwaysinline_recursively
+; CHECK-NEXT: call void @ext_func() #1
+  call void @test_calls_tree_2() alwaysinline_recursively
+; CHECK-NEXT: call void @ext_func() #1
+; CHECK-NEXT: call void @ext_func() #1
+; CHECK-NEXT: call void @ext_func() #1
+  ret void
+; CHECK-NEXT: ret void
+}
+
+define void @test_calls_tree_1() {
+  call void @ext_func()
+  ret void
+}
+
+define void @test_calls_tree_2() {
+  call void @test_calls_tree_2_1()
+  call void @test_calls_tree_2_2()
+  call void @test_calls_tree_2_3()
+  ret void
+}
+
+define void @test_calls_tree_2_1() {
+call void @ext_func()
+ret void
+}
+
+define void @test_calls_tree_2_2() {
+call void @ext_func()
+ret void
+}
+
+define void @test_calls_tree_2_3() {
+call void @ext_func()
+ret void
+}
+
+; Test that functions marked noinline are not inlined
+; when annotating call-sites
+
+define void @test_calls_noinline() {
+; CHECK-LABEL: @test_calls_noinline() {
+  call void @test_calls_noinline_inlined() alwaysinline_recursively
+; CHECK-NEXT: call void @ext_func() #1
+  call void @test_calls_noinline_inlined2() alwaysinline_recursively
+; CHECK-NEXT: call void @test_calls_noinline_notinlined()
+  ret void
+; CHECK-NEXT: ret void
+}
+
+define void @test_calls_noinline_inlined() {
+  call void @ext_func()
+  ret void
+}
+
+define void @test_calls_noinline_inlined2() {
+  call void @test_calls_noinline_notinlined()
+  ret void
+}
+
+define void @test_calls_noinline_notinlined() noinline {
+call void @ext_func()
+ret void
+}
+
+; Test that a recursive call is not inlined
+; when annotating call-sites
+
+de

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion added a comment.

Thanks for the ping. I hadn't looked at this since, but I'll update the patch 
this weekend.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.
Herald added a subscriber: kerbowa.

[reverse ping] Is this still active?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

We need more tests here.

For one, the flatten attribute is not necessary to pass the test.
Second, we need to check the corner cases, e.g. reduction with different cycle 
lengths.




Comment at: llvm/docs/LangRef.rst:1428
 can prove that the call/invoke cannot call a convergent function.
+``flatten``
+This attribute is similar to ``alwaysinline``, but applies recursively to

LevitatingLion wrote:
> arsenm wrote:
> > It's not obvious to me what the flatten name means. flatteninline? 
> > recursive_alwaysinline? Something else?
> I agree. What about always_inline_recurse or always_inline_recursively?
I'd prefer `always_inline_recursively` or `recursive_alwaysinline` so far. 
Though something shorter would be fine too.
`always_inline_rec` maybe?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion added inline comments.



Comment at: llvm/docs/LangRef.rst:1428
 can prove that the call/invoke cannot call a convergent function.
+``flatten``
+This attribute is similar to ``alwaysinline``, but applies recursively to

arsenm wrote:
> It's not obvious to me what the flatten name means. flatteninline? 
> recursive_alwaysinline? Something else?
I agree. What about always_inline_recurse or always_inline_recursively?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for working on this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/docs/LangRef.rst:1428
 can prove that the call/invoke cannot call a convergent function.
+``flatten``
+This attribute is similar to ``alwaysinline``, but applies recursively to

It's not obvious to me what the flatten name means. flatteninline? 
recursive_alwaysinline? Something else?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70366/new/

https://reviews.llvm.org/D70366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

[LevitatingLion via Phabricator via cfe-commits]

LevitatingLion created this revision.
LevitatingLion added reviewers: jdoerfert, pcc, chandlerc.
LevitatingLion added projects: LLVM, clang.
Herald added subscribers: dexonsmith, steven_wu, haicheng, hiraditya, eraman, 
nhaehnle, jvesely, mehdi_amini, arsenm.

This adds a new 'flatten' attribute, which works like 'always_inline' but 
applies recursively to inlined call sites. The addition was briefly discussed 
on the mailing list: 
http://lists.llvm.org/pipermail/llvm-dev/2019-November/136514.html

This patch also contains changes to clang, so that it uses the new LLVM 
attribute on functions marked with the clang attribute 'flatten'. Previously, 
clang marked all calls in such functions with 'always_inline'; in effect, only 
the first level of calls was inlined.

Currently this patch fails the '/llvm/test/Bitcode/highLevelStructure.3.2.ll' 
test. llvm-dis seems to be unable to correctly decode attributes stored in the 
bitcode when the new attribute is added, although other attributes don't seem 
to have required any handling of this problem, see 
https://reviews.llvm.org/D62766 or https://reviews.llvm.org/D49165. I 
speculated that's because this is the 65th attribute, so a bitmask indicating 
all attributes doesn't fit in 64 bit anymore.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70366

Files:
  clang/lib/CodeGen/CGCall.cpp
  llvm/docs/BitCodeFormat.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Target/AMDGPU/AMDGPUInline.cpp
  llvm/lib/Target/Hexagon/HexagonLoopIdiomRecognition.cpp
  llvm/lib/Transforms/IPO/AlwaysInliner.cpp
  llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
  llvm/lib/Transforms/IPO/HotColdSplitting.cpp
  llvm/lib/Transforms/IPO/Inliner.cpp
  llvm/lib/Transforms/IPO/PartialInlining.cpp
  llvm/lib/Transforms/IPO/SyntheticCountsPropagation.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/Transforms/Inline/flatten.ll

Index: llvm/test/Transforms/Inline/flatten.ll
===
--- /dev/null
+++ llvm/test/Transforms/Inline/flatten.ll
@@ -0,0 +1,29 @@
+; RUN: opt < %s -inline -S | FileCheck %s
+
+declare void @ext()
+
+define void @should_inline2() {
+  call void @ext()
+  ret void
+}
+
+define void @should_inline() {
+  call void @should_inline2()
+  ret void
+}
+
+define void @should_not_inline() noinline {
+  call void @ext()
+  ret void
+}
+
+; CHECK: @flattened() {
+define void @flattened() {
+; CHECK-NEXT; @ext() #1
+  call void @should_inline() flatten
+; CHECK-NEXT; @should_not_inline() #1
+  call void @should_not_inline() flatten
+  ret void
+}
+
+; CHECK: attributes #1 = { flatten }
Index: llvm/lib/Transforms/Utils/CodeExtractor.cpp
===
--- llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -888,6 +888,7 @@
   // Those attributes should be safe to propagate to the extracted function.
   case Attribute::AlwaysInline:
   case Attribute::Cold:
+  case Attribute::Flatten:
   case Attribute::NoRecurse:
   case Attribute::InlineHint:
   case Attribute::MinSize:
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -697,7 +697,8 @@
   // have its address taken. Doing so would create an undefined external ref to
   // the function, which would fail to link.
   if (HasAvailableExternallyLinkage &&
-  F->hasFnAttribute(Attribute::AlwaysInline))
+  (F->hasFnAttribute(Attribute::AlwaysInline) ||
+   F->hasFnAttribute(Attribute::Flatten)))
 return false;
 
   // Prohibit function address recording if the function is both internal and
Index: llvm/lib/Transforms/IPO/SyntheticCountsPropagation.cpp
===
--- llvm/lib/Transforms/IPO/SyntheticCountsPropagation.cpp
+++ llvm/lib/Transforms/IPO/SyntheticCountsPropagation.cpp
@@ -77,6 +77,7 @@
 if (F.isDeclaration())
   continue;
 if (F.hasFnAttribute(Attribute::AlwaysInline) ||
+F.hasFnAttribute(Attribute::Flatten) ||
 F.hasFnAttribute(Attribute::InlineHint)) {
   // Use a higher value for inline functions to account for the fact that
   // these are usually beneficial to inline.
Index: llvm/lib/Transforms/IPO/PartialInlining.cpp
===
--- llvm/lib/Transforms/IPO/Pa