[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:5496
+  // execution of the await_suspend. To achieve this, we need to prevent the
+  // await_suspend get inlined before CoroSplit pass.
+  //
----------------
Suggestion:

> The `await_suspend` call performed by `co_await` is essentially asynchronous
> to the execution of the coroutine.  Inlining it normally into an unsplit coroutine
> can cause miscompilation because the coroutine CFG misrepresents the true
> control flow of the program: things that happen in the `await_suspend` are not
> guaranteed to happen prior to the resumption of the coroutine, and things that
> happen after the resumption of the coroutine (including its exit and the
> potential deallocation of the coroutine frame) are not guaranteed to happen
> only after the end of `await_suspend`.
>
> The short-term solution to this problem is to mark the call as uninlinable.
> But we don't want to do this if the call is known to be trivial, which is very
> common.

We don't need to get into the long-term solution here, but I'd suggest a pattern
like:

```
  call @llvm.coro.await_suspend(token %suspend, ptr %awaiter, ptr @awaitSuspendFn)
```

and then we just replace that with the normal call during function splitting.  I guess we'd have to make sure we did everything right with the return values and exceptions, which might be a pain.  But it does have the really nice property that we could move all of this "is it trivial" analysis into the LLVM passes: if we decide that it's safe to inline the function before splitting (e.g. `awaitSuspendFn` doesn't contain any uses of `this`), then the optimization is as simple as doing the replacement in CoroEarly instead of after splitting.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:146
+// We can't use (CoroutineSuspendExpr).getCommonExpr()->getType() directly
+// since its type may be AutoType, ElaboratedType, ...
+class AwaiterTypeFinder : public TypeVisitor<AwaiterTypeFinder> {
----------------
Please just do `getNonReferenceType()->getAsCXXRecordDecl()` and conservatively say it might escape if that's null.  And you really don't need to repeat the well-formedness checks about the awaiter type that Sema is presumably already going to have done; that's just asking for extra work if this code ever diverges from Sema, e.g. if the language standard changes.  So you should be able to do all of this without a visitor.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:165
+    // to give users better user experience. It doesn't matter with the
+    // correctness but 1 byte memory overhead.
+#ifdef NDEBUG
----------------
I'm sorry, but I'm really confused about what you're doing here.  In general, we really don't want compiler behavior to change based on whether we're running a release version of the compiler.  And this would be disabling the optimization completely in release builds?

Are you maybe trying to check whether we're doing an unoptimized build of the *program*?  That's `CodeGenOpts.OptimizationLevel == 0`.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:351
 
+  bool maySuspendLeakCoroutineHandle() const {
+    return isCoroutine() && CurCoro.MaySuspendLeak;
----------------
(1) I'd prefer that we use the term "escape" over "leak" here.
(2) None of these bugs require us to escape the coroutine handle.

Maybe `mustPreventInliningOfAwaitSuspend`?  It's not really a general property.


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

https://reviews.llvm.org/D157833