[Coroutines] Set presplit attribute in Clang and mlir

[ChuanqiXu]

Summary

See https://reviews.llvm.org/D100282 and https://github.com/llvm/llvm-project/issues/49264 for the background.
Simply, coroutine shouldn't be inlined before CoroSplit. And the marker for pre-splited coroutine is created in CoroEarly pass, which ran after AlwaysInliner Pass in O0 pipeline. So that the AlwaysInliner couldn't detect it shouldn't inline a coroutine. So here is the error.

This patch set the presplit attribute in clang and mlir. So the inliner would always detect the attribute before splitting.

[aeubanks]

pass manager changes seem fine, but I'll let somebody else lgtm

llvm/test/Other/new-pm-O0-defaults.ll

34

should have -NEXT

[ChuanqiXu]

llvm/test/Other/new-pm-O0-defaults.ll

34

In case there is no CHECK-DIS, CHECK-CORO-NEXT doesn't make sense.

[rjmccall]

I don't find it at all weird to say that there's an invariant that certain intrinsics can only appear in functions with a particular attribute, and so the frontend has a responsibility to set the attribute if it's using that intrinsic. We do the same thing with several other intrinsics, such as the constrained FP intrinsics generated for #pragma fenv_access.

[ChuanqiXu]

Address the comments to set the attribute in frontend.

[ChuanqiXu]

@ezhulenev hi, we are discussing if we should set the coroutine attributes in middle-end or front-end. Now in this revision, it would set the coroutine.presplit attribute in frontend, which would break the MLIR test. I have tried to implement it myself today. But I found that it is really not easy to edit MLIR's code. I found that you introduced the Coroutines intrinsic into MLIR. How do you think about set the attribute in the frontend of MLIR? And if you approve this, could you help to add the attributes in MLIR? We could still add the attribute in CoroEarly pass for a short time to make things consistent.

[ezhulenev]

There are two places where in MLIR you can put an attribute to coroutine functions:

1. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp#L126

This is the point when coroutine functions are created, and you can attach attribute to the func argument

2. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp#L325

Another options is to attach attribute to the op->getParentOfType<FuncOp> when async runtime operations lowered to coro.id intrinsic.

/cc @mehdi_amini to chime in what option is better. I'm not sure though what type of MLIR attribute will be translated to LLVM "coroutine.presplit"="0", UnitAttr? Or it must be IntegerAttr.

I'm on vacation with limjted access to computer until the end of the year, I can take a look at it myself ~late Dec or early Jan.

[ChuanqiXu]

In D115790#3199905, @ezhulenev wrote:

There are two places where in MLIR you can put an attribute to coroutine functions:

1. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp#L126

This is the point when coroutine functions are created, and you can attach attribute to the func argument

2. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp#L325

Another options is to attach attribute to the op->getParentOfType<FuncOp> when async runtime operations lowered to coro.id intrinsic.

/cc @mehdi_amini to chime in what option is better. I'm not sure though what type of MLIR attribute will be translated to LLVM "coroutine.presplit"="0", UnitAttr? Or it must be IntegerAttr.

I'm on vacation with limjted access to computer until the end of the year, I can take a look at it myself ~late Dec or early Jan.

Hi, thanks for looking into this. In this func, https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp#L126, I tried to write:

func.setAttr("coroutine.presplit", "0");

But it shows that we could only set the attribute for argument and result type instead of the function itself. How could we make it?

[ChuanqiXu]

Set presplit attribute in mlir too

[ChuanqiXu]

@ezhulenev I finally made it! Thanks for your help! @mehdi_amini @ftynse hi, might you help to look if the change in mlir part is good?

[ChuanqiXu]

Clean codes.

[ezhulenev]

mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp

201 ↗

(On Diff #395963)

nit: builder has an API to simplify attributes construction: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/Builders.h#L92-L100

[ChuanqiXu]

Address comments.

[ChuanqiXu]

@rjmccall gentle ping~

[rjmccall]

clang/lib/CodeGen/CGCoroutine.cpp

712 ↗

(On Diff #396254)

The assertion here is not necessary; if it was, we'd need it everywhere.

Please add a comment like "LLVM expects coroutines to have this attribute set coming out of the frontend."

llvm/docs/Coroutines.rst

1179 ↗

(On Diff #396254)

Please add this paragraph (modified appropriately) to all of the other coro.id.* intrinsics.

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

198 ↗

(On Diff #396254)

This should turn into the same assertion as above. (Swift will need a change, but that's Swift's problem, and I'll take care of it.)

llvm/lib/Transforms/Coroutines/CoroInternal.h

42 ↗

(On Diff #396254)

Editorial: don't put "the" before LLVM here

Since this change requires frontend cooperation, and the refactor will *also* require frontend cooperation, should we just do this in one step so that frontends are less disrupted?

[ChuanqiXu]

Address comments.

[rjmccall]

Alright, that's fine.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroEarly.cpp

198 ↗

(On Diff #396254)

I don't add the assertion here since I am worrying it may break some builds. Would you be happy to add the assertion when you change swift?

llvm/lib/Transforms/Coroutines/CoroInternal.h

42 ↗

(On Diff #396254)

I didn't image swift before when I wrote the FIXME. In case we only need to care about clang and mlir, I think it doesn't matter a lot. But it matters if there is other frontends.
I also found that the values "UNPREPARED_FOR_SPLIT"，“PREPARED_FOR_SPLIT” and "ASYNC_RESTART_AFTER_SPLIT" are only meaningful under LegacyPM. @aeubanks , hi what's the status of LegacyPM? Do we need to care about the correctness for the LegacyPM? Or we could give up on it?

[MaskRay]

Early heads-up: we see a failure with llvm/lib/Transforms/Coroutines/CoroEarly.cpp:186 in bool (anonymous namespace)::Lowerer::lowerEarlyIntrinsics(llvm::Function &): F.hasFnAttribute(CORO_PRESPLIT_ATTR) && F.getFnAttribute(CORO_PRESPLIT_ATTR).getValueAsString() == UNPREPARED_FOR_SPLIT && "The frontend uses Swtich-Resumed ABI should emit " "\"coroutine.presplit\" attribute with value \"0\" for the " "coroutine."

A colleague may follow up with the issue or request a revert.

[ChuanqiXu]

In D115790#3221307, @MaskRay wrote:

Early heads-up: we see a failure with llvm/lib/Transforms/Coroutines/CoroEarly.cpp:186 in bool (anonymous namespace)::Lowerer::lowerEarlyIntrinsics(llvm::Function &): F.hasFnAttribute(CORO_PRESPLIT_ATTR) && F.getFnAttribute(CORO_PRESPLIT_ATTR).getValueAsString() == UNPREPARED_FOR_SPLIT && "The frontend uses Swtich-Resumed ABI should emit " "\"coroutine.presplit\" attribute with value \"0\" for the " "coroutine."

A colleague may follow up with the issue or request a revert.

Thanks for reporting this. I have tested this locally with our workloads. I guess there are 3 reasons to hit this:
(1) The frontend (clang or mlir) is updated.
(2) The input IR is generated by an old frontend.
(3) The tested frontend uses switched-resume ABI coroutine intrinsics and the frontend isn't clang nor mlir.

clang/include/clang/Basic/DiagnosticGroups.td

def CompoundTokenSplit : DiagGroup<"compound-token-split",
                                    [CompoundTokenSplitByMacro,
                                     CompoundTokenSplitBySpace]>;
 def CoroutineMissingUnhandledException :
   DiagGroup<"coroutine-missing-unhandled-exception">;
 def DeprecatedExperimentalCoroutine :
   DiagGroup<"deprecated-experimental-coroutine">;
 def DeprecatedCoroutine :
   DiagGroup<"deprecated-coroutine", [DeprecatedExperimentalCoroutine]>;
-def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException, DeprecatedCoroutine]>;
+def AlwaysInlineCoroutine :
+  DiagGroup<"always-inline-coroutine">;
+def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException, DeprecatedCoroutine, AlwaysInlineCoroutine]>;
 def ObjCBoolConstantConversion : DiagGroup<"objc-bool-constant-conversion">;
 def ConstantConversion : DiagGroup<"constant-conversion",
                                    [BitFieldConstantConversion,
                                     ObjCBoolConstantConversion]>;
 def LiteralConversion : DiagGroup<"literal-conversion">;
 def StringConversion : DiagGroup<"string-conversion">;
 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "return type of 'coroutine_handle<>::address should be 'void*' (have %0) in order to get capability with existing async C API.">,
   InGroup<Coroutine>;
 def err_coroutine_promise_final_suspend_requires_nothrow : Error<
   "the expression 'co_await __promise.final_suspend()' is required to be non-throwing"
 >;
 def note_coroutine_function_declare_noexcept : Note<
   "must be declared with 'noexcept'"
 >;
+def warn_always_inline_coroutine : Warning<
+  "A coroutine marked always_inline might not be inlined properly."
+  >,
+  InGroup<AlwaysInlineCoroutine>;
 } // end of coroutines issue category
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Nullability Issue" in {

clang/lib/Sema/SemaCoroutine.cpp

void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   if (!Fn->CoroutinePromise)
     return FD->setInvalidDecl();
 
   if (isa<CoroutineBodyStmt>(Body)) {
     // Nothing todo. the body is already a transformed coroutine body statement.
     return;
   }
 
+  // The coroutine marked always inline might not be inlined properly in current
+  // compiler support. Only the ramp function is guarantted to be inlined. It
+  // might be different to what users expects to. Emit a warning to tell it.
+  if (FD->hasAttr<AlwaysInlineAttr>())
+    Diag(FD->getLocation(), diag::warn_always_inline_coroutine);
+
   // Coroutines [stmt.return]p1:
   //   A return statement shall not appear in a coroutine.
   if (Fn->FirstReturnLoc.isValid()) {
     assert(Fn->FirstCoroutineStmtLoc.isValid() &&
                    "first coroutine location not set");
     Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
     Diag(Fn->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
             << Fn->getFirstCoroutineStmtKeyword();

clang/test/CodeGen/lto-newpm-pipeline.c

 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null -fexperimental-new-pass-manager -fdebug-pass-manager -flto=thin -Os %s 2>&1 | FileCheck %s \
 // RUN:   -check-prefix=CHECK-THIN-OPTIMIZED
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null -fexperimental-new-pass-manager -fdebug-pass-manager -flto=full -Oz %s 2>&1 | FileCheck %s \
 // RUN:   -check-prefix=CHECK-FULL-OPTIMIZED
 // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null -fexperimental-new-pass-manager -fdebug-pass-manager -flto=thin -Oz %s 2>&1 | FileCheck %s \
 // RUN:   -check-prefix=CHECK-THIN-OPTIMIZED
 
 // CHECK-FULL-O0: Running pass: AlwaysInlinerPass
-// CHECK-FULL-O0-NEXT: Running analysis: InnerAnalysisManagerProxy
 // CHECK-FULL-O0-NEXT: Running analysis: ProfileSummaryAnalysis
+// CHECK-FULL-O0-NEXT: Running analysis: InnerAnalysisManagerProxy
+// CHECK-FULL-O0-NEXT: Running analysis: LazyCallGraphAnalysis
 // CHECK-FULL-O0: Running pass: CoroSplitPass
 // CHECK-FULL-O0-NEXT: Running pass: CoroCleanupPass
 // CHECK-FULL-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-FULL-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-FULL-O0-NEXT: Running pass: AnnotationRemarksPass
 // CHECK-FULL-O0-NEXT: Running pass: VerifierPass
 // CHECK-FULL-O0-NEXT: Running analysis: VerifierAnalysis
 // CHECK-FULL-O0-NEXT: Running pass: BitcodeWriterPass
 
 // CHECK-THIN-O0: Running pass: AlwaysInlinerPass
-// CHECK-THIN-O0-NEXT: Running analysis: InnerAnalysisManagerProxy
 // CHECK-THIN-O0-NEXT: Running analysis: ProfileSummaryAnalysis
+// CHECK-THIN-O0-NEXT: Running analysis: InnerAnalysisManagerProxy
+// CHECK-THIN-O0-NEXT: Running analysis: LazyCallGraphAnalysis
 // CHECK-THIN-O0: Running pass: CoroCleanupPass
 // CHECK-THIN-O0-NEXT: Running pass: CanonicalizeAliasesPass
 // CHECK-THIN-O0-NEXT: Running pass: NameAnonGlobalPass
 // CHECK-THIN-O0-NEXT: Running pass: AnnotationRemarksPass
 // CHECK-THIN-O0-NEXT: Running pass: VerifierPass
 // CHECK-THIN-O0-NEXT: Running analysis: VerifierAnalysis
 // CHECK-THIN-O0-NEXT: Running pass: ThinLTOBitcodeWriterPass
 

clang/test/CodeGenCoroutines/coro-always-inline.cpp

 // CHECK: [[CAST1:%[0-9]+]] = bitcast %"struct.std::awaitable"* %ref.tmp{{.*}} to i8*
 // CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 1, i8* [[CAST1]])
 
 // CHECK: [[CAST2:%[0-9]+]] = bitcast %"struct.std::awaitable"* %ref.tmp{{.*}} to i8*
 // CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 1, i8* [[CAST2]])
 // CHECK: [[CAST3:%[0-9]+]] = bitcast %"struct.std::awaitable"* %ref.tmp{{.*}} to i8*
 // CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 1, i8* [[CAST3]])
 void foo() { co_return; }
+
+// check that bar is not inlined even it's marked as always_inline
+
+// CHECK-LABEL:   define {{.*}} void @_Z3bazv()
+// CHECK:         call void @_Z3barv(
+__attribute__((__always_inline__)) void bar() {
+  co_return;
+}
+void baz() {
+  bar();
+  co_return;
+}

clang/test/SemaCXX/coroutines.cpp

struct missing_await_resume {
   void await_suspend(std::coroutine_handle<>);
 };
 
 void test_missing_awaitable_members() {
   co_await missing_await_ready{}; // expected-error {{no member named 'await_ready' in 'missing_await_ready'}}
   co_await missing_await_suspend{}; // expected-error {{no member named 'await_suspend' in 'missing_await_suspend'}}
   co_await missing_await_resume{}; // expected-error {{no member named 'await_resume' in 'missing_await_resume'}}
 }
+
+__attribute__((__always_inline__)) void warn_always_inline() { // expected-warning {{A coroutine marked always_inline might not be inlined properly}}
+  co_await suspend_always{};
+}

llvm/lib/Passes/PassBuilderPipelines.cpp

for (auto &C : PipelineStartEPCallbacks)
     C(MPM, Level);
 
   if (PGOOpt && PGOOpt->DebugInfoForProfiling)
     MPM.addPass(createModuleToFunctionPassAdaptor(AddDiscriminatorsPass()));
 
   for (auto &C : PipelineEarlySimplificationEPCallbacks)
     C(MPM, Level);
 
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
+
   // Build a minimal pipeline based on the semantics required by LLVM,
   // which is just that always inlining occurs. Further, disable generating
   // lifetime intrinsics to avoid enabling further optimizations during
   // code generation.
   MPM.addPass(AlwaysInlinerPass(
       /*InsertLifetimeIntrinsics=*/false));
 
   if (PTO.MergeFunctions)
ModulePassManager PassBuilder::buildO0DefaultPipeline(OptimizationLevel Level,
   if (!VectorizerStartEPCallbacks.empty()) {
     FunctionPassManager FPM;
     for (auto &C : VectorizerStartEPCallbacks)
       C(FPM, Level);
     if (!FPM.isEmpty())
       MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
   }
 
-  MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
   CGSCCPassManager CGPM;
   CGPM.addPass(CoroSplitPass());
   MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
   MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
 
   for (auto &C : OptimizerLastEPCallbacks)
     C(MPM, Level);
 

llvm/test/Other/new-pass-manager.ll

 ; RUN:     | FileCheck %s --check-prefix=CHECK-CALLGRAPH
 ; CHECK-CALLGRAPH: Running pass: RequireAnalysisPass
 ; CHECK-CALLGRAPH: Running analysis: CallGraphAnalysis
 
 ; RUN: opt -disable-output -disable-verify -verify-cfg-preserved=1 -debug-pass-manager \
 ; RUN:     -passes='default<O0>' %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-O0 --check-prefix=%llvmcheckext
 ; CHECK-O0: Running pass: AlwaysInlinerPass
-; CHECK-O0-NEXT: Running analysis: InnerAnalysisManagerProxy<{{.*}}>
 ; CHECK-O0-NEXT: Running analysis: ProfileSummaryAnalysis
+; CHECK-O0-NEXT: Running analysis: InnerAnalysisManagerProxy<{{.*}}>
+; CHECK-O0-NEXT: Running analysis: LazyCallGraphAnalysis
 ; CHECK-EXT-NEXT: Running pass: {{.*}}Bye
 ; We don't have checks for CHECK-NOEXT here, but this simplifies the test, while
 ; avoiding FileCheck complaining about the unused prefix.
 ; CHECK-NOEXT: {{.*}}
 
 ; RUN: opt -disable-output -disable-verify -verify-cfg-preserved=1 -debug-pass-manager \
 ; RUN:     -passes='repeat<3>(no-op-module)' %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-REPEAT-MODULE-PASS

llvm/test/Other/new-pm-O0-defaults.ll

 ; RUN:     -passes='thinlto<O0>' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK,CHECK-THINLTO
 ; RUN: opt -disable-verify -verify-cfg-preserved=0 -debug-pass-manager \
 ; RUN:     -passes='lto<O0>' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK,CHECK-LTO
 
 ; CHECK-DIS: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-DIS-NEXT: Running pass: AddDiscriminatorsPass
+; CHECK-CORO: Running pass: CoroEarlyPass
 ; CHECK-DIS-NEXT: Running pass: AlwaysInlinerPass
 ; CHECK-DIS-NEXT: Running analysis: ProfileSummaryAnalysis
-; CHECK-DEFAULT: Running pass: AlwaysInlinerPass
-; CHECK-DEFAULT-NEXT: Running analysis: InnerAnalysisManagerProxy
+; CHECK-DEFAULT-NEXT: Running pass: AlwaysInlinerPass
 ; CHECK-DEFAULT-NEXT: Running analysis: ProfileSummaryAnalysis
 ; CHECK-MATRIX: Running pass: LowerMatrixIntrinsicsPass
 ; CHECK-MATRIX-NEXT: Running analysis: TargetIRAnalysis
-; CHECK-CORO-NEXT: Running pass: CoroEarlyPass
 ; CHECK-CORO-NEXT: Running analysis: InnerAnalysisManagerProxy
 ; CHECK-CORO-NEXT: Running analysis: LazyCallGraphAnalysis
 ; CHECK-CORO-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-CORO-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
 ; CHECK-CORO-NEXT: Running analysis: OuterAnalysisManagerProxy
 ; CHECK-CORO-NEXT: Running pass: CoroSplitPass
 ; CHECK-CORO-NEXT: Running pass: CoroCleanupPass
 ; CHECK-PRE-LINK: Running pass: CanonicalizeAliasesPass