[Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[lxfind]

Summary

In generating the code for symmetric transfer, a temporary object is created to store the returned handle from await_suspend() call of the awaiter. Previously this temp won't be cleaned up until very later, which ends up causing this temp to be spilled to the heap. However, we know that this temp will no longer be needed after we obtained the address. We can clean it up right after the call to address member.

[junparser]

Thanks for the change. LGTM, and testcase？

[lxfind]

In D87470#2267060, @junparser wrote:

Thanks for the change. LGTM, and testcase？

Not sure how to add a test case for this though. We don't seem to explicitly test AST output.

[rjmccall]

Also LGTM with a testcase. It's fine to test the result of IRGen.

[lxfind]

Add test case. Verify that the size of the frame reduced by 8.

[lxfind]

hmm @rjmccall, I don't think there is a stable way to test this. The code generated for symmetric transfer is way too complicated to stably pattern match one less item in the frame.

[lxfind]

Remove unstable test

[rjmccall]

In D87470#2268267, @lxfind wrote:

hmm @rjmccall, I don't think there is a stable way to test this. The code generated for symmetric transfer is way too complicated to stably pattern match one less item in the frame.

I don't know that much about the code-generation here, but when cleanups escape into a surrounding scope, you can usually test that the cleanup is emitted before some call tied to a later statement in that scope. You don't have to test for every last instruction in the function.

[lxfind]

Add test to verify that lifetime.end appears right after the address call.

[lxfind]

remove asan option, not needed

[rjmccall]

Thanks, LGTM.

[lxfind]

In D87470#2268911, @rjmccall wrote:

Thanks, LGTM.

Thank you for reviewing and the suggestions on testcase!

[junparser]

@lxfind , could you backport this to branch 11?

[lxfind]

In D87470#2273310, @junparser wrote:

@lxfind , could you backport this to branch 11?

I am actually seeing some problems with this change. Still investigating.

clang/lib/Sema/SemaCoroutine.cpp

static Expr *maybeTailCall(Sema &S, QualType RetType, Expr *E,
   Expr *JustAddress = AddressExpr.get();
 
   // Check that the type of AddressExpr is void*
   if (!JustAddress->getType().getTypePtr()->isVoidPointerType())
     S.Diag(cast<CallExpr>(JustAddress)->getCalleeDecl()->getLocation(),
            diag::warn_coroutine_handle_address_invalid_return_type)
         << JustAddress->getType();
 
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-                          JustAddress);
+  return S.MaybeCreateExprWithCleanups(
+      buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume, JustAddress));
 }
 
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await
 /// expression.
 static ReadySuspendResumeResult buildCoawaitCalls(Sema &S, VarDecl *CoroPromise,
                                                   SourceLocation Loc, Expr *E) {
   OpaqueValueExpr *Operand = new (S.Context)
       OpaqueValueExpr(Loc, E->getType(), VK_LValue, E->getObjectKind(), E);