[coroutine] should disable inline before calling coro split

[lxfind]

Summary

This is a rework of D85812, which didn't land.
When callee coroutine function is inlined into caller coroutine function before coro-split pass, llvm will emits "coroutine should have exactly one defining @llvm.coro.begin". It seems that coro-early pass can not handle this quiet well.
So we believe that unsplited coroutine function should not be inlined.
This patch fix such issue by not inlining function if it has attribute "coroutine.presplit" (it means the function has not been splited) to fix this issue
test plan: check-llvm, check-clang

In D85812, there was suggestions on moving the macros to Attributes.td to avoid circular header dependency issue.
I believe it's not worth doing just to be able to use one constant string in one place.
Today, there are already 3 possible attribute values for "coroutine.presplit": https://github.com/llvm/llvm-project/blob/c6543cc6b8f107b58e7205d8fc64865a508bacba/llvm/lib/Transforms/Coroutines/CoroInternal.h#L40-L42
If we move them into Attributes.td, we would be adding 3 new attributes to EnumAttr, just to support this, which I think is an overkill.

Instead, I think the best way to do this is to add an API in Function class that checks whether this function is a coroutine, by checking the attribute by name directly.

[wenlei]

I believe it's not worth doing just to be able to use one constant string in one place.
If we move them into Attributes.td, we would be adding 3 new attributes to EnumAttr, just to support this, which I think is an overkill.

Agreed that string is fine. There're many precedences where string is used in getFnAttribute/hasFnAttribute already, otherwise we will need to be consistent.

llvm/include/llvm/IR/Function.h

272

What is the definition of a coroutine function here? The main coroutine function alone, or does that include split funclets? A function is consider a coroutine function only before corosplit, but no longer after split?

I'm not sure if presplit attribute is the best way to tell whether function is coroutine because the attribute is added, removed in the process of codegen only to reflect split state. And it doesn't look like an invariant. Also what we need here in the context of inline check is not whether a function is coroutine, but wether a function is a presplit coroutine, right?

llvm/lib/Analysis/InlineCost.cpp

2339

This works with the current definition of isCoroutine which is based on split. But the naming is misleading because it may lead people to think that we can't inline any coroutine.

[dongAxis1944]

llvm/include/llvm/IR/Function.h

272

I do not think it is the best way to use the string coroutine.presplit instead of the macro CORO_PRESPLIT_ATTR.

The other parts in LLVM uses CORO_PRESPLIT_ATTR, but not directly use the constant string. so I think it might keep it same.

[lxfind]

llvm/include/llvm/IR/Function.h

272

@dongAxis1944 Where would you suggest to put the definition of CORO_PRESPLIT_ATTR? From what I can see, it seems common in LLVM to use string attribute directly (not saying it's good, but there doesn't seem to be an infrastructure to make this better).

[lxfind]

Add missing file. Rename isCoroutine to isPresplitCoroutine

[junparser]

hi @lxfind, sorry for the late reply. After D85812, we think it might be better to move both of attribute and intrinsic of coroutine into core library, however such changes are too heavy to do.
The reason I suggest adding the attribute into EnumAttr is that i believe that we may need to use presplit attribute in other places (such as lower debug intrinsic. ) and we do not want to write the string anywhere cross different parts. Add API in Function will reduce the impact, so LGTM, thanks for the patch.

[aeubanks]

llvm/test/Transforms/Coroutines/coro-inline.ll

1

Are these -barriers necessary? The test passes without them. And no other test in all of LLVM uses it.

2

-enable-new-pm shouldn't be used to run the NPM, this should be something like -passes=always-inline,coro-early,coro-split.

32

seems like this test should be reduced, is any of this debug info metadata necessary for the test?

[lxfind]

llvm/test/Transforms/Coroutines/coro-inline.ll

1

@dongAxis1944, do you know the context on why this was initially added?

[dongAxis1944]

llvm/test/Transforms/Coroutines/coro-inline.ll

1

this is because we want to run always_inline, then coro-early, split at last. using barrier, will keep this sequence well.

[aeubanks]

llvm/test/Transforms/Coroutines/coro-inline.ll

1

Does it matter if pass A runs on all SCCs, then pass B runs on all SCCs, or if for each SCC, pass A runs then pass B runs before going to the next SCC?

[dongAxis1944]

llvm/test/Transforms/Coroutines/coro-inline.ll

1

the actual actions is that llvm will do always inline for all scc, before doing early and split, so we add barrier.

llvm/include/llvm/IR/Function.h

public:
   }
 
   /// Remove function attribute from this function.
   void removeFnAttr(StringRef Kind) {
     setAttributes(getAttributes().removeAttribute(
         getContext(), AttributeList::FunctionIndex, Kind));
   }
 
+  /// A function will have the "coroutine.presplit" attribute if it's
+  /// a coroutine and has not gone through full CoroSplit pass.
+  bool isPresplitCoroutine() const {
+    return hasFnAttribute("coroutine.presplit");
+  }
+
   enum ProfileCountType { PCT_Invalid, PCT_Real, PCT_Synthetic };
 
   /// Class to represent profile counts.
   ///
   /// This class represents both real and synthetic profile counts.
   class ProfileCount {
   private:
     uint64_t Count;

llvm/lib/Analysis/InlineCost.cpp

 Optional<InlineResult> llvm::getAttributeBasedInliningDecision(
     CallBase &Call, Function *Callee, TargetTransformInfo &CalleeTTI,
     function_ref<const TargetLibraryInfo &(Function &)> GetTLI) {
 
   // Cannot inline indirect calls.
   if (!Callee)
     return InlineResult::failure("indirect call");
 
+  // 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 (Callee->isPresplitCoroutine())
+    return InlineResult::failure("unsplited coroutine call");
+
   // Never inline calls with byval arguments that does not have the alloca
   // address space. Since byval arguments can be replaced with a copy to an
   // alloca, the inlined code would need to be adjusted to handle that the
   // argument is in the alloca address space (so it is a little bit complicated
   // to solve).
   unsigned AllocaAS = Callee->getParent()->getDataLayout().getAllocaAddrSpace();
   for (unsigned I = 0, E = Call.arg_size(); I != E; ++I)
     if (Call.isByValArgument(I)) {

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

PreservedAnalyses AlwaysInlinerPass::run(Module &M,
   auto GetAssumptionCache = [&](Function &F) -> AssumptionCache & {
     return FAM.getResult<AssumptionAnalysis>(F);
   };
   auto &PSI = MAM.getResult<ProfileSummaryAnalysis>(M);
 
   SmallSetVector<CallBase *, 16> Calls;
   bool Changed = false;
   SmallVector<Function *, 16> InlinedFunctions;
-  for (Function &F : 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()) {
       Calls.clear();
 
       for (User *U : F.users())
         if (auto *CB = dyn_cast<CallBase>(U))
           if (CB->getCalledFunction() == &F)
             Calls.insert(CB);
if (!F.isDeclaration() && F.hasFnAttribute(Attribute::AlwaysInline) &&
         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();
   });
 
   // Delete the non-comdat ones from the module and also from our vector.

 InlineCost AlwaysInlinerLegacyPass::getInlineCost(CallBase &CB) {
   Function *Callee = CB.getCalledFunction();
 
   // Only inline direct calls to functions with always-inline attributes
   // that are viable for inlining.
   if (!Callee)
     return InlineCost::getNever("indirect call");
 
+  // 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 (Callee->isPresplitCoroutine())
+    return InlineCost::getNever("unsplited coroutine call");
+
   // FIXME: We shouldn't even get here for declarations.
   if (Callee->isDeclaration())
     return InlineCost::getNever("no definition");
 
   if (!CB.hasFnAttr(Attribute::AlwaysInline))
     return InlineCost::getNever("no alwaysinline attribute");
 
   auto IsViable = isInlineViable(*Callee);
   if (!IsViable.isSuccess())
     return InlineCost::getNever(IsViable.getFailureReason());
 
   return InlineCost::getAlways("always inliner");
 }

llvm/test/Transforms/Coroutines/Inputs/sample.text.prof

+ff:152730084:141806
+  1: 123
+
+foo:152730084:141806
+  65492: ff:302659

llvm/test/Transforms/Coroutines/coro-inline.ll

+; RUN: opt < %s -always-inline -barrier -coro-early -barrier -coro-split -S | FileCheck %s
+; RUN: opt < %s -enable-new-pm  -always-inline -coro-early -coro-split  -S | FileCheck %s
+; RUN: opt < %s -sample-profile-file=%S/Inputs/sample.text.prof -pgo-kind=pgo-sample-use-pipeline -coro-early -barrier -sample-profile -barrier -coro-split  -disable-inlining=true -S | FileCheck %s
+; RUN: opt < %s -enable-new-pm -sample-profile-file=%S/Inputs/sample.text.prof -pgo-kind=pgo-sample-use-pipeline -coro-early -sample-profile -coro-split  -disable-inlining=true -S | FileCheck %s
+
+; Function Attrs: alwaysinline ssp uwtable
+define void @ff() #0 !dbg !12 {
+entry:
+  %id = call token @llvm.coro.id(i32 16, i8* null, i8* null, i8* null)
+  %begin = call i8* @llvm.coro.begin(token %id, i8* null)
+  ret void
+}
+
+; CHECK:  call void @ff()
+; Function Attrs: alwaysinline ssp uwtable
+define void @foo() #0 !dbg !8 {
+entry:
+  %id1 = call token @llvm.coro.id(i32 16, i8* null, i8* null, i8* null)
+  %begin = call i8* @llvm.coro.begin(token %id1, i8* null)
+  call void @ff(), !dbg !11
+  ret void
+}
+
+declare token @llvm.coro.id(i32, i8* readnone, i8* nocapture readonly, i8*)
+declare i8* @llvm.coro.begin(token, i8* writeonly)
+
+attributes #0 = { alwaysinline ssp uwtable "coroutine.presplit"="1" "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="penryn" "target-features"="+cx16,+cx8,+fxsr,+mmx,+sahf,+sse,+sse2,+sse3,+sse4.1,+ssse3,+x87" "unsafe-fp-math"="false" "use-sample-profile" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "inline_O2.cpp", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!8 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 46, type: !9, scopeLine: 46, flags: DIFlagPrototyped, unit: !0, retainedNodes: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!11 = !DILocation(line: 2, column: 0, scope: !8)
+!12 = distinct !DISubprogram(name: "ff", linkageName: "ff", scope: !1, file: !1, line: 46, type: !9, scopeLine: 46, flags: DIFlagPrototyped, unit: !0, retainedNodes: !2)