[clang] [llvm] [coro] Lower `llvm.coro.await.suspend.handle` to resume with tail call (PR #89751)

[via cfe-commits]

================
@@ -1056,6 +1083,25 @@ void CoroCloner::create() {
   // Set up the new entry block.
   replaceEntryBlock();
 
+  // Turn symmetric transfers into musttail calls.
+  for (CallInst *ResumeCall : Shape.SymmetricTransfers) {
+    ResumeCall = cast<CallInst>(VMap[ResumeCall]);
+    ResumeCall->setCallingConv(NewF->getCallingConv());
+    if (TTI.supportsTailCallFor(ResumeCall)) {
+      // FIXME: Could we support symmetric transfer effectively without
+      // musttail?
+      ResumeCall->setTailCallKind(CallInst::TCK_MustTail);
+    }
+
+    // Put a 'ret void' after the call, and split any remaining instructions to
----------------
zmodem wrote:

> Sorry for insisting on this

Not at all, I want us to resolve this, not paper over it. :)

>  it's maybe because I got "bitten" before (with the suspend)

But in that case the problem was kind of the opposite, right? Instructions between the `resume` and `suspend` blocked the musttail optimization. The problem was not whether those instructions would have been executed or not, but that they were inserted in a place where it wasn't allowed due to special constraints on this intrinsic.

The idea with my patch is to eliminate that problem by not having special restrictions about instructions after the intrinsic, they just won't be executed because control continues in the resumed function and doesn't come back.

> what other examples do we have where, silently, instructions don't get executed after a call?

This happens any time a call doesn't return.

> Also, maybe this would become moot if we address https://discourse.llvm.org/t/coro-pre-split-handling-of-the-suspend-edge/75043 like @jyknight suggested (i.e. not even have the misleading edge)?

Yes that's probably true, but that's a bigger change and I think my patch is a simpler solution to this specific problem: now that we have an intrinsic that corresponds directly to the symmetric transfer, let's just lower it as such directly.

> If I read correctly, @zmodem said he'd like to mention this in the doc or check it by assertions or verifiers. So it looks consensus to me?

No, I said that *if* we decide that instructions after `llvm.coro.await.suspend.handle` are not allowed, we should document and check it.

I still don't think we should do that, since it's a difficult invariant to maintain. We'd have to teach passes to respect it, keep updating a list of harmless instructions that are allowed (debug/lifetime intrinsics, ...) and we'd end up chasing down those assert/verifier failures forever.

With the current patch, we don't have to teach other passes anything. They already cannot assume that an unknown intrinsic call will return.

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