[PATCH] D117965: [AlwaysInliner] Enable call site inlining to make flatten attribute working again (PR53360)

2022-01-25 Thread Arthur Eubanks via Phabricator via cfe-commits
aeubanks added a comment.

sounds good for the purposes of going back to what we had before, but we should 
reconsider the attribute support




Comment at: llvm/lib/Transforms/IPO/AlwaysInliner.cpp:95
 
-  // Remember to try and delete this function afterward. This both avoids
-  // re-walking the rest of the module and avoids dealing with any iterator
-  // invalidation issues while deleting functions.
-  InlinedFunctions.push_back();
+  if (F.hasFnAttribute(Attribute::AlwaysInline))
+// Remember to try and delete this function afterward. This both avoids

xbolva00 wrote:
> avoid DCE of internal functions without always_inline; check test pr2945 in 
> always-inline.ll
no need to change in this patch, but it looks like that was a past limitation, 
removing any dead internal function should be fine (subject to comdats). either 
we should not delete any function or we should attempt to remove any function 
we can



Comment at: llvm/test/Transforms/Inline/always-inline.ll:150
 
+define i32 @outer7b() {
+; CHECK-LABEL: @outer7b(

s/CHECK-CALL/CHECK/g should be good enough? any reason for outer7b? it's 
exactly the same as outer7a


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117965

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


[PATCH] D117965: [AlwaysInliner] Enable call site inlining to make flatten attribute working again (PR53360)

2022-01-25 Thread Dávid Bolvanský via Phabricator via cfe-commits
xbolva00 added a comment.

There is also second use case which I mentioned and I am interested in.

>> I'd vote for properly implementing [1] or removing our support for flatten.

I would rather prefer  to restore atleast something which was working before 
new PM and then improve it, instead of full removal thanks to oversight during 
new PM transition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117965

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


[PATCH] D117965: [AlwaysInliner] Enable call site inlining to make flatten attribute working again (PR53360)

2022-01-25 Thread Arthur Eubanks via Phabricator via cfe-commits
aeubanks added a comment.

The existing implementation only inlines top level call sites, which doesn't 
match gcc where all calls are recursively inlined [1]. gcc's implementation 
makes more sense IMO, only inlining top level call sites doesn't seem super 
useful.

I'd vote for properly implementing [1] or removing our support for flatten.

[1] https://lists.llvm.org/pipermail/llvm-dev/2019-November/136514.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117965

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


[PATCH] D117965: [AlwaysInliner] Enable call site inlining to make flatten attribute working again (PR53360)

2022-01-25 Thread Dávid Bolvanský via Phabricator via cfe-commits
xbolva00 added a comment.

Aiming this fix for LLVM 14.0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117965

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


[PATCH] D117965: [AlwaysInliner] Enable call site inlining to make flatten attribute working again (PR53360)

2022-01-22 Thread Dávid Bolvanský via Phabricator via cfe-commits
xbolva00 added inline comments.



Comment at: llvm/lib/Transforms/IPO/AlwaysInliner.cpp:95
 
-  // Remember to try and delete this function afterward. This both avoids
-  // re-walking the rest of the module and avoids dealing with any iterator
-  // invalidation issues while deleting functions.
-  InlinedFunctions.push_back();
+  if (F.hasFnAttribute(Attribute::AlwaysInline))
+// Remember to try and delete this function afterward. This both avoids

avoid DCE of internal functions without always_inline; check test pr2945 in 
always-inline.ll


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117965

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


[PATCH] D117965: [AlwaysInliner] Enable call site inlining to make flatten attribute working again (PR53360)

2022-01-22 Thread Dávid Bolvanský via Phabricator via cfe-commits
xbolva00 created this revision.
xbolva00 added reviewers: leonardchan, aeubanks.
Herald added subscribers: ormris, hiraditya.
xbolva00 requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Problem: Migration to new PM broke flatten attribute.

This is one use case why LLVM should support inlining call-site with 
alwaysinline.  The flatten attribute is nowdays broken, so we should either 
land patch like this one or remove everything related to  flatten attribute 
from Clang.

Second use case is something like "per call site inlining intrinsics" to 
control inlining even more; mentioned in
https://lists.llvm.org/pipermail/cfe-dev/2018-September/059232.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117965

Files:
  clang/test/CodeGen/flatten.c
  clang/test/CodeGenCXX/flatten.cpp
  llvm/lib/Transforms/IPO/AlwaysInliner.cpp
  llvm/test/Transforms/Inline/always-inline.ll

Index: llvm/test/Transforms/Inline/always-inline.ll
===
--- llvm/test/Transforms/Inline/always-inline.ll
+++ llvm/test/Transforms/Inline/always-inline.ll
@@ -1,14 +1,7 @@
-; RUN: opt < %s -inline-threshold=0 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
-;
-; Ensure the threshold has no impact on these decisions.
-; RUN: opt < %s -inline-threshold=2000 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
-; RUN: opt < %s -inline-threshold=-2000 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
-;
-; The new pass manager doesn't re-use any threshold based infrastructure for
-; the always inliner, but test that we get the correct result. The new PM
-; always inliner also doesn't support inlining call-site alwaysinline
-; annotations. It isn't clear that this is a reasonable use case for
-; 'alwaysinline'.
+; Ensure the threshold has no impact on these decisions. The new pass manager
+; doesn't re-use any threshold based infrastructure for the always inliner,
+; but test that we get the correct result.
+
 ; RUN: opt < %s -inline-threshold=0 -passes=always-inline -S | FileCheck %s --check-prefix=CHECK
 ; RUN: opt < %s -inline-threshold=2000 -passes=always-inline -S | FileCheck %s --check-prefix=CHECK
 ; RUN: opt < %s -inline-threshold=-2000 -passes=always-inline -S | FileCheck %s --check-prefix=CHECK
@@ -145,15 +138,24 @@
 ; CHECK-LABEL: @inner7(
   ret i32 1
 }
-define i32 @outer7() {
-; CHECK-CALL-LABEL: @outer7(
-; CHECK-CALL-NOT: call
-; CHECK-CALL: ret
+define i32 @outer7a() {
+; CHECK-LABEL: @outer7a(
+; CHECK-NOT: call
+; CHECK: ret
 
%r = call i32 @inner7() alwaysinline
ret i32 %r
 }
 
+define i32 @outer7b() {
+; CHECK-LABEL: @outer7b(
+; CHECK-NOT: call
+; CHECK: ret
+
+   %r = call i32 @inner7() #0
+   ret i32 %r
+}
+
 define internal float* @inner8(float* nocapture align 128 %a) alwaysinline {
 ; CHECK-NOT: @inner8(
   ret float* %a
@@ -318,3 +320,5 @@
   call void @inner14()
   ret void
 }
+
+attributes #0 = { alwaysinline }
Index: llvm/lib/Transforms/IPO/AlwaysInliner.cpp
===
--- llvm/lib/Transforms/IPO/AlwaysInliner.cpp
+++ llvm/lib/Transforms/IPO/AlwaysInliner.cpp
@@ -54,13 +54,13 @@
 if (F.isPresplitCoroutine())
   continue;
 
-if (!F.isDeclaration() && F.hasFnAttribute(Attribute::AlwaysInline) &&
-isInlineViable(F).isSuccess()) {
+if (!F.isDeclaration() && isInlineViable(F).isSuccess()) {
   Calls.clear();
 
   for (User *U : F.users())
 if (auto *CB = dyn_cast(U))
-  if (CB->getCalledFunction() == )
+  if (CB->getCalledFunction() ==  &&
+  CB->hasFnAttr(Attribute::AlwaysInline))
 Calls.insert(CB);
 
   for (CallBase *CB : Calls) {
@@ -92,10 +92,11 @@
 Changed = true;
   }
 
-  // Remember to try and delete this function afterward. This both avoids
-  // re-walking the rest of the module and avoids dealing with any iterator
-  // invalidation issues while deleting functions.
-  InlinedFunctions.push_back();
+  if (F.hasFnAttribute(Attribute::AlwaysInline))
+// Remember to try and delete this function afterward. This both avoids
+// re-walking the rest of the module and avoids dealing with any
+// iterator invalidation issues while deleting functions.
+InlinedFunctions.push_back();
 }
   }
 
Index: clang/test/CodeGenCXX/flatten.cpp
===
--- clang/test/CodeGenCXX/flatten.cpp
+++ clang/test/CodeGenCXX/flatten.cpp
@@ -1,7 +1,3 @@
-// UNSUPPORTED: experimental-new-pass-manager
-// See the comment for CodeGen/flatten.c on why this is unsupported with the new
-// PM.
-
 // RUN: %clang_cc1 -triple=x86_64-linux-gnu -std=c++11 %s -emit-llvm -o -