[coroutine] should disable inline before calling coro split

[dongAxis1944]

Summary

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

[mtrofin]

The "repository" of this change seems incorrect. It should be "rG LLVM Github Monorepo", instead it's "rZORG LLVM Github Zorg". I wonder if that's why the context of the changes isn't available?

[dongAxis1944]

update new diff for showing the context of the changes

[dongAxis1944]

In D85812#2219584, @mtrofin wrote:

The "repository" of this change seems incorrect. It should be "rG LLVM Github Monorepo", instead it's "rZORG LLVM Github Zorg". I wonder if that's why the context of the changes isn't available?

sorry for that, i forgot to commit context of the changed.

[wenlei]

You'd also want to prevent inlining presplit coroutine in sample profile loader (SampleProfileLoader::inlineHotFunctions), which is before CGSCC pipeline and does top-down inlining now.

llvm/lib/Analysis/InlineCost.cpp

2393

I can see the problem with always inliner as that is outside of CGSCC pipeline, and caller could be processed before callee. However, do you actually run into this problem with CGSCC inliner? I'd expect callee to be split already as they should be processed earlier in the pipeline.

[wenlei]

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

[dongAxis1944]

In D85812#2220117, @wenlei wrote:

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

yes, my change in 'llvm::getInlineCost' was for sample profile loader inline, not for CGSCC. I think you might be right, i will do my change in always inline and sample profile loader inline. Thank you for your advice.

[junparser]

In D85812#2220117, @wenlei wrote:

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

Yep. Since corosplit pass runs before CGSCC inline, this should not happen for CGSCC inline.

[dongAxis1944]

move the check from llvm::getInlineCost to SampleProfileLoader::inlineHotFunctions and AlwaysInlinerLegacyPass::getInlineCost

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

1072 ↗

(On Diff #286192)

Move this into inlineCallInstruction? inlineCallInstruction has another call site on line 1059. Would be good to add a comment too, for sample loader and always inline.

[dongAxis1944]

move check to SampleProfileLoader::inlineCallInstruction

[wenlei]

LGTM, thanks for the fix.

[MaskRay]

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

[MaskRay]

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

2

You may consider split-file %s %t to place sample.text.prof in this file.

[MaskRay]

llvm/include/llvm/Transforms/Coroutines.h

24

The code motion was not needed, I guess?

It probably should have used constexpr instead of macros...

[wenlei]

In D85812#2234313, @MaskRay wrote:

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

[dongAxis1944]

In D85812#2237812, @wenlei wrote:

In D85812#2234313, @MaskRay wrote:

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

if i put the attribute CORO_PRESPLIT_ATTR to the attribute.td, it seem it might not have the layering issue(Because it will not depend coroutine lib itself), do you think it might be better?

[wenlei]

In D85812#2243570, @dongAxis1944 wrote:

In D85812#2237812, @wenlei wrote:

In D85812#2234313, @MaskRay wrote:

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

if i put the attribute CORO_PRESPLIT_ATTR to the attribute.td, it seem it might not have the layering issue(Because it will not depend coroutine lib itself), do you think it might be better?

For attribute.td, it will be automatically checked for inlining through hasCompatibleFnAttrs. The CompatRule<"isEqual .. rule checks whether the caller and callee has the same attribute value, which may be different from what you need. There maybe a way to write a compatible rule that fits your need? I'm not familiar with how to do that though (or if that's possible)..

[dongAxis1944]

hello, i just update previous patch, if it is ok, i will commit the code

[dongAxis1944]

llvm/include/llvm/Transforms/Coroutines.h

24

yes, but i want to use another patch to resolve it. We prepare to move this constant to attribute.td

[dongAxis1944]

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

2

how to do it? do you have any examples?

[MaskRay]

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

2

Search for split-file %s in llvm/test. There are many examples.

[wenlei]

Took another look, AlwaysInline is part of LLVMipo too. :(

To abide the layering, this attribute check for AlwaysInline also needs to be in InlineCost, which is in Analysis lib. I think legality checks should be shared between SCC Inline and Always Inline, but that'd be a refactoring by itself. Moving it to attribute.td is another way to do it (need to make sure hasCompatibleFnAttrs works properly). If you plan to move it to attribute.td always, it might make sense to combine the change so layering isn't an issue for this change.

On the other hand, I do see string literal being used for string attributes (example: "no-builtins")...

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

2

You may consider split-file %s %t to place sample.text.prof in this file.

I would prefer to keep sample.text.prof in a separate file as is, so the file itself is a legal profile. This is also more consistent with how other test cases use profile as input.

[lxfind]

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

[junparser]

In D85812#2406785, @lxfind wrote:

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

For now，we do not have time to refactor this. For this case, we can move all attributes of coroutine to LLVMCore library. If you do have anytime, feel free to do this.

llvm/include/llvm/Transforms/Coroutines.h

 
 namespace llvm {
 
+// CoroEarly pass marks every function that has coro.begin with a string
+// attribute "coroutine.presplit"="0". CoroSplit pass processes the coroutine
+// twice. First, it lets it go through complete IPO optimization pipeline as a
+// single function. It forces restart of the pipeline by inserting an indirect
+// call to an empty function "coro.devirt.trigger" which is devirtualized by
+// CoroElide pass that triggers a restart of the pipeline by CGPassManager.
+// When CoroSplit pass sees the same coroutine the second time, it splits it up,
+// adds coroutine subfunctions to the SCC to be processed by IPO pipeline.
+#define CORO_PRESPLIT_ATTR "coroutine.presplit"
+#define UNPREPARED_FOR_SPLIT "0"
+#define PREPARED_FOR_SPLIT "1"
+
 class Pass;
 class PassManagerBuilder;
 

llvm/lib/Analysis/InlineCost.cpp

 #include "llvm/IR/Operator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Coroutines.h"
 
 using namespace llvm;
 

   if (!Callee)
     return llvm::InlineCost::getNever("indirect call");
 
+  if (Callee->hasFnAttribute(CORO_PRESPLIT_ATTR))
+      return llvm::InlineCost::getNever("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

llvm/lib/Transforms/Coroutines/CoroInternal.h

 void initializeCoroElidePass(PassRegistry &);
 void initializeCoroCleanupPass(PassRegistry &);
 
-// CoroEarly pass marks every function that has coro.begin with a string
-// attribute "coroutine.presplit"="0". CoroSplit pass processes the coroutine
-// twice. First, it lets it go through complete IPO optimization pipeline as a
-// single function. It forces restart of the pipeline by inserting an indirect
-// call to an empty function "coro.devirt.trigger" which is devirtualized by
-// CoroElide pass that triggers a restart of the pipeline by CGPassManager.
-// When CoroSplit pass sees the same coroutine the second time, it splits it up,
-// adds coroutine subfunctions to the SCC to be processed by IPO pipeline.
-
-#define CORO_PRESPLIT_ATTR "coroutine.presplit"
-#define UNPREPARED_FOR_SPLIT "0"
-#define PREPARED_FOR_SPLIT "1"
-
 #define CORO_DEVIRT_TRIGGER_FN "coro.devirt.trigger"
 
 namespace coro {

llvm/lib/Transforms/IPO/AlwaysInliner.cpp

 #include "llvm/Transforms/IPO/Inliner.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
+#include "llvm/Transforms/Coroutines.h"
 
 using namespace llvm;
 

   SmallSetVector<CallSite, 16> Calls;
   bool Changed = false;
   SmallVector<Function *, 16> InlinedFunctions;
-  for (Function &F : M)
+  for (Function &F : M) {
+    if (F.hasFnAttribute(CORO_PRESPLIT_ATTR))
+      continue;
+
     if (!F.isDeclaration() && F.hasFnAttribute(Attribute::AlwaysInline) &&
         isInlineViable(F)) {
       Calls.clear();

       // invalidation issues while deleting functions.
       InlinedFunctions.push_back(&F);
     }
+  }
 
   // Remove any live functions.
   erase_if(InlinedFunctions, [&](Function *F) {

 InlineCost AlwaysInlinerLegacyPass::getInlineCost(CallSite CS) {
   Function *Callee = CS.getCalledFunction();
 
+  if (!Callee)
+      return InlineCost::getNever("always inliner");
+
+  if (Callee->hasFnAttribute(CORO_PRESPLIT_ATTR))
+      return InlineCost::getNever("unsplited coroutine call");
+
   // Only inline direct calls to functions with always-inline attributes
   // that are viable for inlining. FIXME: We shouldn't even get here for
   // declarations.
-  if (Callee && !Callee->isDeclaration() &&
+  if (!Callee->isDeclaration() &&
       CS.hasFnAttr(Attribute::AlwaysInline) && isInlineViable(*Callee))
     return InlineCost::getAlways("always inliner");
 

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

+ff:152730084:141806
+  1: 123
+
+foo:152730084:141806
+  65492: ff:302659
 No newline at end of file

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

+; RUN: opt < %s -always-inline -barrier -coro-early -barrier -coro-split -S | FileCheck %s
+; RUN: opt < %s -sample-profile-file=%S/Inputs/sample.text.prof -coro-early -barrier -sample-profile -barrier -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)