[libcxx-commits] [clang] [libcxx] Elide suspension points via [[clang::coro_await_suspend_destroy]] (PR #152623)

[via libcxx-commits]

================
@@ -401,36 +432,85 @@ static ReadySuspendResumeResult buildCoawaitCalls(Sema &S, VarDecl *CoroPromise,
     return Calls;
   }
   Expr *CoroHandle = CoroHandleRes.get();
+  Calls.UseAwaitSuspendDestroy = false;
   CallExpr *AwaitSuspend = cast_or_null<CallExpr>(
       BuildSubExpr(ACT::ACT_Suspend, "await_suspend", CoroHandle));
   if (!AwaitSuspend)
     return Calls;
+
+  // When this `await_suspend()` overload is annotated with
+  // `[[clang::coro_await_suspend_destroy]]`, do NOT call `await_suspend()` --
+  // instead call `await_suspend_destroy(Promise&)`.  This assumes that the
+  // `await_suspend()` is just a compatibility stub consisting of:
+  //     await_suspend_destroy(handle.promise());
+  //     handle.destroy();
+  // Users of the attribute must follow this contract.  Then, diagnostics from
+  // both `await_suspend` and `await_suspend_destroy` will get exposed.
+  CallExpr *PlainAwaitSuspend = nullptr;
+  if (FunctionDecl *AwaitSuspendCallee = AwaitSuspend->getDirectCallee()) {
+    if (AwaitSuspendCallee->hasAttr<CoroAwaitSuspendDestroyAttr>()) {
+      Calls.UseAwaitSuspendDestroy = true;
+      ExprResult PromiseRefRes =
+          buildPromiseRef(S, CoroPromise->getType(), Loc);
+      if (PromiseRefRes.isInvalid()) {
+        Calls.IsInvalid = true;
+        return Calls;
+      }
+      Expr *PromiseRef = PromiseRefRes.get();
+      PlainAwaitSuspend = AwaitSuspend;
+      AwaitSuspend = cast_or_null<CallExpr>(
+          BuildSubExpr(ACT::ACT_Suspend, "await_suspend_destroy", PromiseRef));
+      if (!AwaitSuspend)
+        return Calls;
+    }
+  }
----------------
snarkmaster wrote:

> emit await_suspend directly, instead of via `coro.await_suspend` intrinsics

I agree it's a nice, clean idea. This is sort of what I tried in the very first version of this (https://github.com/llvm/llvm-project/pull/152029), which I quickly put into "draft" mode because I realized my implementation was wrong.

Here's my old thinking, which led to wrong / broken code:

 - `await_suspend()` must contain an `h.destroy()` call since we want portability to compilers without the attribute.

 - I wanted to make the `await_suspend` wrapper omit `coro.suspend` -- my goal here is to make it look like a non-suspending coroutine so it optimizes like a plain function.

 - The `h.destroy()` ultimately invokes a `coro.destroy` intrinsic. The implementation of that intrinsic does an indirect call through a `destroy` pointer that is written to the coro frame via one of the intrinsics that I elided. That frame setup would also write `resume` pointer which is required by `h.done()`.
 
 - So, now the `h.destroy()` call will fail with a null pointer dereference.

I tried a few permutations, but at the time I didn't understand the code well enough to try the right one, perhaps.

Are you saying that specifically **the following** version will both maintain the internal invariants, and optimize well?

- Keep `coro.save` and `coro.suspend`
- Replace `coro.awaits.suspend.XXX` by a direct `await_suspend`-wrapper call

I am happy to try this out, though I'll be pleasantly surprised all the indirection optimizes away and it emits good code.

@ChuanqiXu9, can you confirm I have the correct idea now? Are there any gotchas?

https://github.com/llvm/llvm-project/pull/152623