[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

[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

[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

[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 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

[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

[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