[coroutines] Allow co_await and co_yield expressions that return an lvalue to compile

[EricWF]

Summary

The title says it all.

[EricWF]

• Fix assertion for co_yield.

[EricWF]

• Remove unneeded assertion.

[EricWF]

• Improve tests

[EricWF]

• Improve test by using regular expressions and [[Name]] where possible.

[GorNishanov]

LGTM

test/CodeGenCoroutines/coro-await.cpp

301

s/chrash/crash
s/a await_resume/an await_resume/

[GorNishanov]

lib/CodeGen/CGCoroutine.cpp

256

#ifndef NDEBUG
...
#endif

[GorNishanov]

LGTM with changes

[GorNishanov]

Added John McCall as he made great suggestions last time I touched emitSuspendExpression

[GorNishanov]

test/CodeGenCoroutines/coro-await.cpp

310

In release compiler, labels won't have friendly names. //CHECK: await2.ready will fail

[EricWF]

test/CodeGenCoroutines/coro-await.cpp

310

I'm not sure I understand. Are you saying that Clang generates different IR depending on how it's built (ie release vs debug)?

[EricWF]

• Address most inline comments.

[EricWF]

• Fix the test when -discard-value-names is present, at least my part of the test.

lib/AST/ExprClassification.cpp

#include "clang/AST/StmtNodes.inc"
   case Expr::AsTypeExprClass:
   case Expr::ObjCIndirectCopyRestoreExprClass:
   case Expr::AtomicExprClass:
   case Expr::CXXFoldExprClass:
   case Expr::ArrayInitLoopExprClass:
   case Expr::ArrayInitIndexExprClass:
   case Expr::NoInitExprClass:
   case Expr::DesignatedInitUpdateExprClass:
-  case Expr::CoyieldExprClass:
     return Cl::CL_PRValue;
 
     // Next come the complicated cases.
   case Expr::SubstNonTypeTemplateParmExprClass:
     return ClassifyInternal(Ctx,
                  cast<SubstNonTypeTemplateParmExpr>(E)->getReplacement());
 
     // C, C++98 [expr.sub]p1: The result is an lvalue of type "T".
case Expr::InitListExprClass:
     // value kind for us, so we only need to fine-tune.
     if (E->isRValue())
       return ClassifyExprValueKind(Lang, E, E->getValueKind());
     assert(cast<InitListExpr>(E)->getNumInits() == 1 &&
            "Only 1-element init lists can be glvalues.");
     return ClassifyInternal(Ctx, cast<InitListExpr>(E)->getInit(0));
 
   case Expr::CoawaitExprClass:
-    return ClassifyInternal(Ctx, cast<CoawaitExpr>(E)->getResumeExpr());
+  case Expr::CoyieldExprClass:
+    return ClassifyInternal(Ctx, cast<CoroutineSuspendExpr>(E)->getResumeExpr());
   }
 
   llvm_unreachable("unhandled expression kind in classification");
 }
 
 /// ClassifyDecl - Return the classification of an expression referencing the
 /// given declaration.
 static Cl::Kinds ClassifyDecl(ASTContext &Ctx, const Decl *D) {

lib/CodeGen/CGCoroutine.cpp

 //  %where-to = call i8 @llvm.coro.suspend(...)
 //  switch i8 %where-to, label %coro.ret [ ; jump to epilogue to suspend
 //    i8 0, label %yield.ready   ; go here when resumed
 //    i8 1, label %yield.cleanup ; go here when destroyed
 //  ]
 //
 //  See llvm's docs/Coroutines.rst for more details.
 //
-static RValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Coro,
+namespace {
+  struct LValueOrRValue {
+    LValue LV;
+    RValue RV;
+  };
+}
+static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Coro,
                                     CoroutineSuspendExpr const &S,
                                     AwaitKind Kind, AggValueSlot aggSlot,
-                                    bool ignoreResult) {
+                                    bool ignoreResult, bool forLValue) {
   auto *E = S.getCommonExpr();
 
   // FIXME: rsmith 5/22/2017. Does it still make sense for us to have a 
   // UO_Coawait at all? As I recall, the only purpose it ever had was to
   // represent a dependent co_await expression that couldn't yet be resolved to
   // a CoawaitExpr. But now we have (and need!) a separate DependentCoawaitExpr
   // node to store unqualified lookup results, it seems that the UnaryOperator
   // portion of the representation serves no purpose (and as seen in this patch,
static LValueOrRValue emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Coro,
   Switch->addCase(Builder.getInt8(1), CleanupBlock);
 
   // Emit cleanup for this suspend point.
   CGF.EmitBlock(CleanupBlock);
   CGF.EmitBranchThroughCleanup(Coro.CleanupJD);
 
   // Emit await_resume expression.
   CGF.EmitBlock(ReadyBlock);
-  return CGF.EmitAnyExpr(S.getResumeExpr(), aggSlot, ignoreResult);
+  assert(isa<CallExpr>(S.getResumeExpr()) || isa<CXXBindTemporaryExpr>(S.getResumeExpr()));
+  LValueOrRValue Res;
+  if (forLValue)
+    Res.LV = CGF.EmitLValue(S.getResumeExpr());
+  else
+    Res.RV = CGF.EmitAnyExpr(S.getResumeExpr(), aggSlot, ignoreResult);
+  return Res;
 }
 
 RValue CodeGenFunction::EmitCoawaitExpr(const CoawaitExpr &E,
                                         AggValueSlot aggSlot,
                                         bool ignoreResult) {
   return emitSuspendExpression(*this, *CurCoro.Data, E,
                                CurCoro.Data->CurrentAwaitKind, aggSlot,
-                               ignoreResult);
+                               ignoreResult, /*forLValue*/false).RV;
 }
 RValue CodeGenFunction::EmitCoyieldExpr(const CoyieldExpr &E,
                                         AggValueSlot aggSlot,
                                         bool ignoreResult) {
   return emitSuspendExpression(*this, *CurCoro.Data, E, AwaitKind::Yield,
-                               aggSlot, ignoreResult);
+                               aggSlot, ignoreResult, /*forLValue*/false).RV;
 }
 
 void CodeGenFunction::EmitCoreturnStmt(CoreturnStmt const &S) {
   ++CurCoro.Data->CoreturnCount;
   EmitStmt(S.getPromiseCall());
   EmitBranchThroughCleanup(CurCoro.Data->FinalJD);
 }
 
+
+static QualType getCoroutineSuspendExprReturnType(const ASTContext &Ctx,
+  const CoroutineSuspendExpr *E) {
+  const auto *RE = E->getResumeExpr();
+  // Is it possible for RE to be a CXXBindTemporaryExpr wrapping
+  // a MemberCallExpr?
+  assert(isa<CallExpr>(RE) && "unexpected suspend expression type");
+  return cast<CallExpr>(RE)->getCallReturnType(Ctx);
+}
+
+LValue
+CodeGenFunction::EmitCoawaitLValue(const CoawaitExpr *E) {
+  assert(getCoroutineSuspendExprReturnType(getContext(), E)->isReferenceType() &&
+         "Can't have a scalar return unless the return type is a "
+         "reference type!");
+  return emitSuspendExpression(*this, *CurCoro.Data, *E,
+                               CurCoro.Data->CurrentAwaitKind, AggValueSlot::ignored(),
+                               /*ignoreResult*/false, /*forLValue*/true).LV;
+}
+
+LValue
+CodeGenFunction::EmitCoyieldLValue(const CoyieldExpr *E) {
+  assert(getCoroutineSuspendExprReturnType(getContext(), E)->isReferenceType() &&
+         "Can't have a scalar return unless the return type is a "
+         "reference type!");
+  return emitSuspendExpression(*this, *CurCoro.Data, *E,
+                               AwaitKind::Yield, AggValueSlot::ignored(),
+                               /*ignoreResult*/false, /*forLValue*/true).LV;
+}
+
 // Hunts for the parameter reference in the parameter copy/move declaration.
 namespace {
 struct GetParamRef : public StmtVisitor<GetParamRef> {
 public:
   DeclRefExpr *Expr = nullptr;
   GetParamRef() {}
   void VisitDeclRefExpr(DeclRefExpr *E) {
     assert(Expr == nullptr && "multilple declref in param move");

lib/CodeGen/CGExpr.cpp

   case Expr::CXXDynamicCastExprClass:
   case Expr::CXXReinterpretCastExprClass:
   case Expr::CXXConstCastExprClass:
   case Expr::ObjCBridgedCastExprClass:
     return EmitCastLValue(cast<CastExpr>(E));
 
   case Expr::MaterializeTemporaryExprClass:
     return EmitMaterializeTemporaryExpr(cast<MaterializeTemporaryExpr>(E));
+
+  case Expr::CoawaitExprClass:
+    return EmitCoawaitLValue(cast<CoawaitExpr>(E));
+  case Expr::CoyieldExprClass:
+    return EmitCoyieldLValue(cast<CoyieldExpr>(E));
   }
 }
 
 /// Given an object of the given canonical type, can we safely copy a
 /// value out of it based on its initializer?
 static bool isConstantEmittableObjectType(QualType type) {
   assert(type.isCanonical());
   assert(!type->isReferenceType());

lib/CodeGen/CodeGenFunction.h

   void EmitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt &S);
   void EmitObjCAutoreleasePoolStmt(const ObjCAutoreleasePoolStmt &S);
 
   void EmitCoroutineBody(const CoroutineBodyStmt &S);
   void EmitCoreturnStmt(const CoreturnStmt &S);
   RValue EmitCoawaitExpr(const CoawaitExpr &E,
                          AggValueSlot aggSlot = AggValueSlot::ignored(),
                          bool ignoreResult = false);
+  LValue EmitCoawaitLValue(const CoawaitExpr *E);
   RValue EmitCoyieldExpr(const CoyieldExpr &E,
                          AggValueSlot aggSlot = AggValueSlot::ignored(),
                          bool ignoreResult = false);
+  LValue EmitCoyieldLValue(const CoyieldExpr *E);
   RValue EmitCoroutineIntrinsic(const CallExpr *E, unsigned int IID);
 
   void EnterCXXTryStmt(const CXXTryStmt &S, bool IsFnTryBlock = false);
   void ExitCXXTryStmt(const CXXTryStmt &S, bool IsFnTryBlock = false);
 
   void EmitCXXTryStmt(const CXXTryStmt &S);
   void EmitSEHTryStmt(const SEHTryStmt &S);
   void EmitSEHLeaveStmt(const SEHLeaveStmt &S);

test/CodeGenCoroutines/coro-await.cpp

-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 \
+// RUN:   -emit-llvm %s -o - -disable-llvm-passes -Wno-coroutine -Wno-unused | FileCheck %s
 
 namespace std {
 namespace experimental {
 template <typename... T>
 struct coroutine_traits;
 
 template <typename Promise = void> struct coroutine_handle;
 

 }
 
 // Verifies that we don't crash when awaiting on an lvalue.
 // CHECK-LABEL: @_Z11AwaitLValuev(
 void AwaitLValue() {
   suspend_always lval;
   co_await lval;
 }
+
+struct AwaitResumeReturnsLValue {
+  bool await_ready();
+  void await_suspend(std::experimental::coroutine_handle<>);
+  int& await_resume();
+};
+
+
+template<>
+struct std::experimental::coroutine_traits<void,double> {
+  struct promise_type {
+    void get_return_object();
+    init_susp initial_suspend();
+    final_susp final_suspend();
+    void return_void();
+    AwaitResumeReturnsLValue yield_value(int);
+  };
+};
+
+// Verifies that we don't chrash when returning an lvalue from a await_resume()
+// expression.
+// CHECK-LABEL:  define void @_Z18AwaitReturnsLValued(double)
+// CHECK-NOT: define
+void AwaitReturnsLValue(double) {
+  AwaitResumeReturnsLValue a;
+  // CHECK: await.ready:
+  // CHECK-NEXT: %call13 = call dereferenceable(4) i32* @_ZN24AwaitResumeReturnsLValue12await_resumeEv(%struct.AwaitResumeReturnsLValue* %a)
+  // CHECK-NEXT: store i32* %call13, i32** %x, align 8
+  int& x = co_await a;
+  // CHECK: await2.ready:
+  // CHECK-NEXT: %call22 = call dereferenceable(4) i32* @_ZN24AwaitResumeReturnsLValue12await_resumeEv(%struct.AwaitResumeReturnsLValue* %ref.tmp14)
+  // CHECK-NEXT: store i32* %call22, i32** %y, align 8
+  int& y = co_await AwaitResumeReturnsLValue{};
+  // CHECK: yield.ready:
+  // CHECK-NEXT: %call32 = call dereferenceable(4) i32* @_ZN24AwaitResumeReturnsLValue12await_resumeEv(%struct.AwaitResumeReturnsLValue* %ref.tmp23)
+  int& z = co_yield 42;
+}