[coroutines] Fix rebuilding of dependent coroutine parameters

[EricWF]

Summary

We were not handling correctly rebuilding of parameter and were not creating copies for them.
Now we will always rebuild parameter moves in TreeTransform's TransformCoroutineBodyStmt.

[GorNishanov]

test/CodeGenCoroutines/coro-params.cpp

103

use %[[x_copy]] instead of %x1 here

[EricWF]

@GorNishanov I think we should be transforming the move parameters, instead of re-building them entirely. I'll put together a different set of changes.

[GorNishanov]

test updated to use %[[copy_x]] instead of %x1

[EricWF]

• Diagnose non-moveable but otherwise unnamed parameters.
• Have TreeTransform.h check if building the move params is successful.

lib/Sema/SemaCoroutine.cpp

1285

@rsmith Is there a better way to check if the move would be valid for unnamed parameters without building up these expressions?

[rsmith]

lib/Sema/SemaCoroutine.cpp

1285

We could wire through a flag to tell AddInitializerToDecl to build the InitializationSequence but not Perform it, but I don't think it's worth the effort. In fact, I'd prefer that we store the copy statement in the AST regardless, and make the choice to elide the copy from within CodeGen. (Sema shouldn't be dropping parts of the AST just because CodeGen doesn't need them; for example, a tool that wants to identify all potential callers of the move constructor should be able to find this call.)

I think we should also disable elision of parameter copies under -fno-elide-constructors.

[EricWF]

• No longer unnamed parameters from the AST.

[EricWF]

lib/Sema/SemaCoroutine.cpp

1285

Makes sense that we shouldn't drop this from the AST. I'll fix that.

lib/Sema/CoroutineStmtBuilder.h

public:
   /// promise type in order to construct.
   ///
   /// For example different new/delete overloads are selected depending on
   /// if the promise type provides `unhandled_exception()`, and therefore they
   /// cannot be built until the promise type is complete so that we can perform
   /// name lookup.
   bool buildDependentStatements();
 
+  /// \brief Build just parameter moves. To use for rebuilding in TreeTransform.
+  bool buildParameterMoves();
+
   bool isInvalid() const { return !this->IsValid; }
 
 private:
   bool makePromiseStmt();
   bool makeInitialAndFinalSuspend();
   bool makeNewAndDeleteExpr();
   bool makeOnFallthrough();
   bool makeOnException();

lib/Sema/SemaCoroutine.cpp

bool CoroutineStmtBuilder::buildDependentStatements() {
   assert(!this->IsPromiseDependentType &&
          "coroutine cannot have a dependent promise type");
   this->IsValid = makeOnException() && makeOnFallthrough() &&
                   makeGroDeclAndReturnStmt() && makeReturnOnAllocFailure() &&
                   makeNewAndDeleteExpr();
   return this->IsValid;
 }
 
+bool CoroutineStmtBuilder::buildParameterMoves() {
+  assert(this->IsValid && "coroutine already invalid");
+  assert(this->ParamMoves.empty() && "param moves already built");
+  return this->IsValid = makeParamMoves();
+}
+
 bool CoroutineStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
       S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
     return false;
 
TypeSourceInfo *TargetLoc =
       S.Context.getTrivialTypeSourceInfo(TargetType, ExprLoc);
 
   return S
       .BuildCXXNamedCast(ExprLoc, tok::kw_static_cast, TargetLoc, E,
                          SourceRange(ExprLoc, ExprLoc), E->getSourceRange())
       .get();
 }
 
+
 /// \brief Build a variable declaration for move parameter.
 static VarDecl *buildVarDecl(Sema &S, SourceLocation Loc, QualType Type,
-                             StringRef Name) {
-  DeclContext *DC = S.CurContext;
-  IdentifierInfo *II = &S.PP.getIdentifierTable().get(Name);
+                             IdentifierInfo *II) {
   TypeSourceInfo *TInfo = S.Context.getTrivialTypeSourceInfo(Type, Loc);
   VarDecl *Decl =
-      VarDecl::Create(S.Context, DC, Loc, Loc, II, Type, TInfo, SC_None);
+      VarDecl::Create(S.Context, S.CurContext, Loc, Loc, II, Type, TInfo, SC_None);
   Decl->setImplicit();
   return Decl;
 }
 
 bool CoroutineStmtBuilder::makeParamMoves() {
   for (auto *paramDecl : FD.parameters()) {
     auto Ty = paramDecl->getType();
     if (Ty->isDependentType())
       continue;
 
     // No need to copy scalars, llvm will take care of them.
     if (Ty->getAsCXXRecordDecl()) {
-      if (!paramDecl->getIdentifier())
-        continue;
-
       ExprResult ParamRef =
           S.BuildDeclRefExpr(paramDecl, paramDecl->getType(),
                              ExprValueKind::VK_LValue, Loc); // FIXME: scope?
       if (ParamRef.isInvalid())
         return false;
 
       Expr *RCast = castForMoving(S, ParamRef.get());
 
-      auto D = buildVarDecl(S, Loc, Ty, paramDecl->getIdentifier()->getName());
-
+      auto D = buildVarDecl(S, Loc, Ty, paramDecl->getIdentifier());
       S.AddInitializerToDecl(D, RCast, /*DirectInit=*/true);
 
       // Convert decl to a statement.
       StmtResult Stmt = S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(D), Loc, Loc);
       if (Stmt.isInvalid())
         return false;
 
       ParamMovesVector.push_back(Stmt.get());
     }
   }
 
   // Convert to ArrayRef in CtorArgs structure that builder inherits from.
   ParamMoves = ParamMovesVector;

lib/Sema/TreeTransform.h

 
     if (auto *ReturnStmt = S->getReturnStmt()) {
       StmtResult Res = getDerived().TransformStmt(ReturnStmt);
       if (Res.isInvalid())
         return StmtError();
       Builder.ReturnStmt = Res.get();
     }
   }
+  if (!Builder.buildParameterMoves())
+    return StmtError();
 
   return getDerived().RebuildCoroutineBodyStmt(Builder);
 }
 
 template<typename Derived>
 StmtResult
 TreeTransform<Derived>::TransformCoreturnStmt(CoreturnStmt *S) {
   ExprResult Result = getDerived().TransformInitializer(S->getOperand(),

test/CodeGenCoroutines/coro-params.cpp

void f(int val, MoveOnly moParam, MoveAndCopy mcParam) {
   // CHECK: call void @_ZN14suspend_always12await_resumeEv(
 
   // Destroy promise, then parameter copies:
   // CHECK: call void @_ZNSt12experimental16coroutine_traitsIJvi8MoveOnly11MoveAndCopyEE12promise_typeD1Ev(%"struct.std::experimental::coroutine_traits<void, int, MoveOnly, MoveAndCopy>::promise_type"* %__promise) #2
   // CHECK-NEXT: call void @_ZN11MoveAndCopyD1Ev(%struct.MoveAndCopy* %[[McCopy]])
   // CHECK-NEXT: call void @_ZN8MoveOnlyD1Ev(%struct.MoveOnly* %[[MoCopy]]
   // CHECK-NEXT: call i8* @llvm.coro.free(
 }
+
+// CHECK-LABEL: void @_Z16dependent_paramsI1A1BEvT_T0_S3_(%struct.A* %x, %struct.B*, %struct.B* %y)
+template <typename T, typename U>
+void dependent_params(T x, U, U y) {
+  // CHECK: %[[x_copy:.+]] = alloca %struct.A
+  // CHECK-NEXT: %[[unnamed_copy:.+]] = alloca %struct.B
+  // CHECK-NEXT: %[[y_copy:.+]] = alloca %struct.B
+
+  // CHECK: call i8* @llvm.coro.begin
+  // CHECK-NEXT: call void @_ZN1AC1EOS_(%struct.A* %[[x_copy]], %struct.A* dereferenceable(512) %x)
+  // CHECK-NEXT: call void @_ZN1BC1EOS_(%struct.B* %[[unnamed_copy]], %struct.B* dereferenceable(512) %0)
+  // CHECK-NEXT: call void @_ZN1BC1EOS_(%struct.B* %[[y_copy]], %struct.B* dereferenceable(512) %y)
+  // CHECK-NEXT: invoke void @_ZNSt12experimental16coroutine_traitsIJv1A1BS2_EE12promise_typeC1Ev(
+
+  co_return;
+}
+
+struct A {
+  int WontFitIntoRegisterForSure[128];
+  A();
+  A(A&&) noexcept;
+  ~A();
+};
+
+struct B {
+  int WontFitIntoRegisterForSure[128];
+  B();
+  B(B&&) noexcept;
+  ~B();
+};
+
+void call_dependent_params() {
+  dependent_params(A{}, B{}, B{});
+}

test/SemaCXX/coroutines.cpp

void test_bad_suspend() {
     await_suspend_type_test<const void> c;
     await_suspend_type_test<const volatile bool> d;
     co_await a; // expected-note {{call to 'await_suspend' implicitly required by coroutine function here}}
     co_await b; // expected-note {{call to 'await_suspend' implicitly required by coroutine function here}}
     co_await c; // OK
     co_await d; // OK
   }
 }
+
+template <int ID = 0>
+struct NoCopy {
+  NoCopy(NoCopy const&) = delete; // expected-note 2 {{deleted here}}
+};
+template <class T, class U>
+void test_dependent_param(T t, U) {
+  // expected-error@-1 {{call to deleted constructor of 'NoCopy<0>'}}
+  // expected-error@-2 {{call to deleted constructor of 'NoCopy<1>'}}
+  ((void)t);
+  co_return 42;
+}
+template void test_dependent_param(NoCopy<0>, NoCopy<1>); // expected-note {{requested here}}