[AlwaysInliner] Enable call site inlining to make flatten attribute working again (#53360)

[xbolva00]

Summary

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

Fixes https://github.com/llvm/llvm-project/issues/53360

[xbolva00]

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

98

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

[xbolva00]

Aiming this fix for LLVM 14.0.

[aeubanks]

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

[xbolva00]

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 user unfriendly removal thanks to oversight during new PM transition.

Imagine also possible use case with those intrinsics:

...
__builtin_flatten(foo());
...

[aeubanks]

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

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

98

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

llvm/test/Transforms/Inline/always-inline.ll

147

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

[xbolva00]

Thanks, addressed review comments.

we should reconsider the attribute support

I agree.

[xbolva00]

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

98

Thanks for info, changed to delete them.

llvm/test/Transforms/Inline/always-inline.ll

147

Yes, simplified.

[aeubanks]

llvm/test/Transforms/Coroutines/coro-retcon-once-private.ll

6

CHECK-NOT: define {{.*}} @f is probably a bit more robust

[thakis]

Could this have caused http://45.33.8.238/linux/66347/step_7.txt ?

[xbolva00]

In D117965#3270744, @thakis wrote:

Could this have caused http://45.33.8.238/linux/66347/step_7.txt ?

Yeah, possible as inliner removes internal unused functions (small change before commit, missed check-clang).

I will revert and fix.

clang/test/CodeGen/flatten.c

-// UNSUPPORTED: experimental-new-pass-manager
-// Currently, different code seems to be intentionally generated under the new
-// PM since we alwaysinline functions and not callsites under new PM.
-// Under new PM, f() will not be inlined from g() since f is not marked as
-// alwaysinline.
-
 // RUN: %clang_cc1 -triple=x86_64-linux-gnu %s -emit-llvm -o - | FileCheck %s
 
 void f(void) {}
 
 __attribute__((noinline)) void ni(void) {}
 
 __attribute__((flatten))
 // CHECK: define{{.*}} void @g()

clang/test/CodeGenCXX/flatten.cpp

-// 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 - | FileCheck %s
 
 void f(void) {}
 
 [[gnu::flatten]]
 // CHECK: define{{.*}} void @_Z1gv()
 void g(void) {
   // CHECK-NOT: call {{.*}} @_Z1fv
   f();
 }

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

PreservedAnalyses AlwaysInlinerPass::run(Module &M,
   for (Function &F : M) {
     // When callee coroutine function is inlined into caller coroutine function
     // before coro-split pass,
     // coro-early pass can not handle this quiet well.
     // So we won't inline the coroutine function if it have not been unsplited
     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<CallBase>(U))
-          if (CB->getCalledFunction() == &F)
+          if (CB->getCalledFunction() == &F &&
+              CB->hasFnAttr(Attribute::AlwaysInline))
             Calls.insert(CB);
 
       for (CallBase *CB : Calls) {
         Function *Caller = CB->getCaller();
         OptimizationRemarkEmitter ORE(Caller);
         auto OIC = shouldInline(
             *CB,
             [&](CallBase &CB) {
if (!F.isDeclaration() && isInlineViable(F).isSuccess()) {
         AttributeFuncs::mergeAttributesForInlining(*Caller, F);
 
         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(&F);
     }
   }
 
   // Remove any live functions.
   erase_if(InlinedFunctions, [&](Function *F) {
     F->removeDeadConstantUsers();
     return !F->isDefTriviallyDead();
   });

llvm/test/Transforms/Coroutines/coro-retcon-once-private.ll

 ; RUN: opt < %s -enable-coroutines -passes='default<O0>' -S | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.12.0"
 
-; CHECK: define internal { i8*, i32 } @f(i8* %buffer, i32* %array)
-; CHECK-NEXT: entry:
-; CHECK-NEXT:  unreachable
+; CHECK-NOT: define {{.*}} @f(i8* %buffer, i32* %array)
 
 define internal {i8*, i32} @f(i8* %buffer, i32* %array) {
 entry:
   %id = call token @llvm.coro.id.retcon.once(i32 8, i32 8, i8* %buffer, i8* bitcast (void (i8*, i1)* @prototype to i8*), i8* bitcast (i8* (i32)* @allocate to i8*), i8* bitcast (void (i8*)* @deallocate to i8*))
   %hdl = call i8* @llvm.coro.begin(token %id, i8* null)
   %load = load i32, i32* %array
   %load.pos = icmp sgt i32 %load, 0
   br i1 %load.pos, label %pos, label %neg

llvm/test/Transforms/Inline/always-inline.ll

-; RUN: opt < %s -inline-threshold=0 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
+; RUN: opt < %s -inline-threshold=0 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK
 ;
 ; Ensure the threshold has no impact on these decisions.
-; RUN: opt < %s -inline-threshold=20000000 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
-; RUN: opt < %s -inline-threshold=-20000000 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
+; RUN: opt < %s -inline-threshold=20000000 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK
+; RUN: opt < %s -inline-threshold=-20000000 -always-inline -enable-new-pm=0 -S | FileCheck %s --check-prefix=CHECK
 ;
 ; 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'.
+; 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=20000000 -passes=always-inline -S | FileCheck %s --check-prefix=CHECK
 ; RUN: opt < %s -inline-threshold=-20000000 -passes=always-inline -S | FileCheck %s --check-prefix=CHECK
 
 define internal i32 @inner1() alwaysinline {
 ; CHECK-NOT: @inner1(
   ret i32 1
 }
 define i32 @outer1() {
 ; CHECK-LABEL: @outer1(
 ; CHECK-NOT: call
 ; CHECK: ret
 
    %r = call i32 @inner1()
    ret i32 %r
 }
 
-; The always inliner can't DCE arbitrary internal functions. PR2945
-define internal i32 @pr2945() nounwind {
-; CHECK-LABEL: @pr2945(
-  ret i32 0
-}
-
 define internal void @inner2(i32 %N) alwaysinline {
 ; CHECK-NOT: @inner2(
   %P = alloca i32, i32 %N
   ret void
 }
 define void @outer2(i32 %N) {
 ; The always inliner (unlike the normal one) should be willing to inline
 ; a function with a dynamic alloca into one without a dynamic alloca.

 }
 
 ; This is not an alwaysinline function and is actually external.
 define i32 @inner7() {
 ; CHECK-LABEL: @inner7(
   ret i32 1
 }
 define i32 @outer7() {
-; CHECK-CALL-LABEL: @outer7(
-; CHECK-CALL-NOT: call
-; CHECK-CALL: ret
-
+; CHECK-LABEL: @outer7(
+; CHECK-NOT: call
+; CHECK: ret
    %r = call i32 @inner7() alwaysinline
    ret i32 %r
 }
 
 define internal float* @inner8(float* nocapture align 128 %a) alwaysinline {
 ; CHECK-NOT: @inner8(
   ret float* %a
 }
 define float @outer8(float* nocapture %a) {
 ; CHECK-LABEL: @outer8(
 ; CHECK-NOT: call float* @inner8
 ; CHECK: ret