[coro]Pass rvalue reference for named local variable to return_value

[tks2103]

Summary

Addressing https://bugs.llvm.org/show_bug.cgi?id=37265.

Implements [class.copy]/33 of coroutines TS

When the criteria for elision of a copy/move operation are met, but not
for an exception-declaration, and the object to be copied is designated by an lvalue, or when the
expression in a return or co_return statement is a (possibly parenthesized) id-expression that
names an object with automatic storage duration declared in the body or
parameter-declaration-clause of the innermost enclosing function or lambda-expression, overload resolution to select
the constructor for the copy or the return_value overload to call is first performed as if the object
were designated by an rvalue.

[tks2103]

This comment was removed by tks2103.

[tks2103]

This comment was removed by tks2103.

[tks2103]

get NRVOCandidate to determine if we should try to move return var

[tks2103]

ping @GorNishanov

[tks2103]

ping @GorNishanov

[tks2103]

ping @GorNishanov SAVE ME GOR! YOU'RE MY ONLY HOPE!

[modocache]

This is great, thanks! Sorry for letting it languish. I defer to @GorNishanov, but I don't see why this couldn't go in now and if there're any edge cases I'm missing we can address those in another patch. I pulled this down and played around with it, and everything seemed OK to me.

I had one nit about formatting, but other than that this is good to go. Do you have commit access? If not, let me know if I can land this for you.

lib/Sema/SemaCoroutine.cpp

851 ↗

(On Diff #164265)

nit: When I run clang-format on this patch, it suggests the following here:

ExprResult MoveResult = this->PerformMoveOrCopyInitialization(
    Entity, NRVOCandidate, E->getType(), E);

Splitting it up as above makes sure this line stays within the 80-character-per-line limit.

[GorNishanov]

LGTM! Thank you for doing this.

[tks2103]

comply with clang-format

[tks2103]

@modocache I do need someone to land this for me! Take it away!

[aaronpuchert]

This change breaks the following code that worked before:

task<MoveOnly&> f(MoveOnly &value) {
  co_return value;
}

The error message is:

clang/test/SemaCXX/coroutine-rvo.cpp:60:13: error: call to deleted constructor of 'MoveOnly'
  co_return value;
            ^~~~~
clang/test/SemaCXX/coroutine-rvo.cpp:43:3: note: 'MoveOnly' has been explicitly marked deleted here
  MoveOnly(const MoveOnly&) = delete;
  ^

Is that maybe intentional, and is the code not intended to compile?

cfe/trunk/lib/Sema/SemaCoroutine.cpp

846

Why not CES_Strict like in Sema::BuildReturnStmt? With CES_Strict the test still works, and we can also return references.

849

The last parameter has type bool, and because we're in if (NRVOCandidate), that will always be true. Wouldn't it be more straightforward to just pass true into the function?

[modocache]

Is that maybe intentional, and is the code not intended to compile?

It looks like it should work to me, but maybe @lewissbaker or @GorNishanov can answer definitively.

cfe/trunk/lib/Sema/SemaCoroutine.cpp

846

So this fixes your test case? If so it sounds good to me. I'll make this change or you can feel free to if you get around to it first.

849

Makes sense! I can send a patch to do this, or feel free to commit one yourself if you get to it first.

[• Quuxplusone]

In D51741#1701757, @aaronpuchert wrote:

This change breaks the following code that worked before:

task<MoveOnly&> f(MoveOnly &value) {
  co_return value;
}

This patch is heavily heavily merge-conflicted by P1825.

Aaron's example code should not be affected by P1825. It should do overload resolution on task<MoveOnly&>::return_value with one parameter of type MoveOnly&.

However,

task<MoveOnly&> g(MoveOnly &&value) {
  co_return value;
}
task<MoveOnly&> h(MoveOnly value) {
  co_return value;
}

Both of these should first do overload resolution for one parameter of type MoveOnly&&, and then, only if that overload resolution fails, should they fall back to overload resolution for one parameter of type MoveOnly&.

[aaronpuchert]

In D51741#1702038, @Quuxplusone wrote:

This patch is heavily heavily merge-conflicted by P1825.

Just to be clear, this change is pretty old: it's already contained in Clang 8. I was just adding comments.

But since you're here: what is the right CopyElisionSemanticsKind, is it CES_AsIfByStdMove like this change does it, or CES_Strict like BuildReturnStmt does it?

cfe/trunk/lib/Sema/SemaCoroutine.cpp

846

I can post a change, I'm just not sure if it's correct. The (Clang) tests run fine, will test with libc++ later today or tomorrow.

857–859

@Quuxplusone Am I right that your paper basically addresses this comment?

[• Quuxplusone]

cfe/trunk/lib/Sema/SemaCoroutine.cpp

857–859

I'm not yet motivated enough to look closely enough to give an informed opinion... but my kneejerk impression is that this FIXME comment is saying "we should do implicit move in the [P1155 sense] here." In which case, this patch (D51741) itself fixed this FIXME at least partly, and maybe completely. Maybe this patch should have removed or amended the FIXME, rather than just adding code above it.

But since you're here: what is the right CopyElisionSemanticsKind, is it CES_AsIfByStdMove like this change does it, or CES_Strict like BuildReturnStmt does it?

Oh geez, asking me about code I introduced... ;) My impression is that the correct thing here is CES_Strict.

Notice that we have a CES_FormerDefault (basically "what were the rules in C++11 before the first DR"), and then a CES_Default (basically "what are the rules in C++17, before P1825"). My -Wreturn-std-move patch added CES_AsIfByStdMove as an implementation detail of the diagnostic codepath. I didn't expect anyone to use it in real code.

However, now that P1825 has been accepted into C++2a, CES_AsIfByStdMove is the actual (draft-)standard behavior, and should rightly be named something like CES_FutureDefault!

So I would suggest that this code should use CES_Strict for now, just like we do in BuildReturnStmt. Then, someone should "implement P1825," which means figuring out what the heck to do about -Wreturn-std-move... but whatever they figure out, it'll apply equally cleanly to this code as to BuildReturnStmt.

I'm confused because both here and BuildReturnStmt have calls to getCopyElisionCandidate outside the call to PerformMoveOrCopyInitialization, even though PerformMoveOrCopyInitialization will happily accept a null NRVOCandidate as a signal to make its own call to getCopyElisionCandidate. (And notice that that call will use CES_Default, which seems right, as opposed to CES_Strict, which seems wrong to me, but clearly I've forgotten how this stuff works.)

[aaronpuchert]

@Quuxplusone Thanks for your very helpful comments!

In which case, this patch (D51741) itself fixed this FIXME at least partly, and maybe completely. Maybe this patch should have removed or amended the FIXME, rather than just adding code above it.

It seems you're right, depending on the CopyElisionSemanticsKind it might even be fixed completely.

However, now that P1825 has been accepted into C++2a, CES_AsIfByStdMove is the actual (draft-)standard behavior, and should rightly be named something like CES_FutureDefault!

Ok, since coroutines are actually a (post-?)C++20 feature, using CES_AsIfByStdMove is not really wrong. But it would be inconsistent with our current treatment of return statements, and maybe we should switch both at the same time. (And perhaps depending on the value of -std=, since enabling coroutines manually is also possible in older standards. Maybe we can have a function that returns that correct value for a given standard?)

So changing the CopyElisionSemanticsKind is not the right fix for my example. Which is not surprising, because the problem isn't that we're eliding a copy that we shouldn't elide, it's that we introduce a local copy which shouldn't be there. (Which might actually turn this code into UB, if the copy constructor was available.) We avoid this with CES_Strict more or less accidentally:

• Without CES_AllowParameters we don't get a candidate because the VarDecl is a parameter.
• Without CES_AllowDifferentTypes, Context.hasSameUnqualifiedType(ReturnType, VDType) returns false and so we return false from Sema::isCopyElisionCandidate, because E->getType() (the type of the DeclRefExpr) is MoveOnly, but the VarDecl is a MoveOnly&.

The actual problem is the call to PerformMoveOrCopyInitialization. For normal return statements, the caller provides us with storage for the return value when that is a struct/class. We then copy (or move) the return value there (RVO) or directly construct it there (NRVO). Returning from a coroutine however just calls some return_value member function. If we look at the existing test, we produce

CoreturnStmt 0x2462f28 <line:61:3, col:13>
|-CXXConstructExpr 0x2462e50 <col:13> 'MoveOnly' 'void (MoveOnly &&) noexcept' elidable
| `-ImplicitCastExpr 0x2462e38 <col:13> 'MoveOnly' xvalue <NoOp>
|   `-DeclRefExpr 0x245e390 <col:13> 'MoveOnly' lvalue Var 0x245e2e8 'value' 'MoveOnly'
`-ExprWithCleanups 0x2462f10 <col:3> 'void'
  `-CXXMemberCallExpr 0x2462ed0 <col:3> 'void'
    |-MemberExpr 0x2462ea0 <col:3> '<bound member function type>' .return_value 0x245fde8
    | `-DeclRefExpr 0x2462e80 <col:3> 'std::experimental::traits_sfinae_base<task<MoveOnly>, void>::promise_type':'task<MoveOnly>::promise_type' lvalue Var 0x245fec8 '__promise' 'std::experimental::traits_sfinae_base<task<MoveOnly>, void>::promise_type':'task<MoveOnly>::promise_type'
    `-MaterializeTemporaryExpr 0x2462ef8 <col:13> 'MoveOnly' xvalue
      `-CXXConstructExpr 0x2462e50 <col:13> 'MoveOnly' 'void (MoveOnly &&) noexcept' elidable
        `-ImplicitCastExpr 0x2462e38 <col:13> 'MoveOnly' xvalue <NoOp>
          `-DeclRefExpr 0x245e390 <col:13> 'MoveOnly' lvalue Var 0x245e2e8 'value' 'MoveOnly'

There is an actual move constructor call. So instead of transforming __promise.return_value(value) into __promise.return_value(std::move(value)), we have transformed it into __promise.return_value(decltype<value>(std::move(value))). This constructor is marked elidable, but since it's not an actual return value, there is nothing we can elide. Indeed, the unoptimized IR (where I've demangled the function names) is:

%ref.tmp9.reload.addr39 = getelementptr inbounds %_Z1fv.Frame, %_Z1fv.Frame* %FramePtr, i32 0, i32 7
%ref.tmp9.reload.addr = getelementptr inbounds %_Z1fv.Frame, %_Z1fv.Frame* %FramePtr, i32 0, i32 7
%value.reload.addr37 = getelementptr inbounds %_Z1fv.Frame, %_Z1fv.Frame* %FramePtr, i32 0, i32 6
call void @"MoveOnly::MoveOnly(MoveOnly&&)"(%struct.MoveOnly* %ref.tmp9.reload.addr39, %struct.MoveOnly* dereferenceable(1) %value.reload.addr37) #2
call void @"task<MoveOnly>::promise_type::return_value(MoveOnly&&)"(%"struct.task<MoveOnly>::promise_type"* %__promise, %struct.MoveOnly* dereferenceable(1) %ref.tmp9.reload.addr)

That move constructor call shouldn't be there. We shouldn't construct anything—if constructor calls are needed, building the call statement will do that. Instead of doing a move or copy initialization, we should do an implicit cast if we're returning a DeclRefExpr referencing a variable that we can consume.

[aaronpuchert]

Please have a look at D68845. This should address the issues that we discussed.

cfe/trunk/lib/Sema/SemaCoroutine.cpp

StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
 
   if (E && E->getType()->isPlaceholderType() &&
       !E->getType()->isSpecificPlaceholderType(BuiltinType::Overload)) {
     ExprResult R = CheckPlaceholderExpr(E);
     if (R.isInvalid()) return StmtError();
     E = R.get();
   }
 
+  // Move the return value if we can
+  if (E) {
+    auto NRVOCandidate = this->getCopyElisionCandidate(E->getType(), E, CES_AsIfByStdMove);
+    if (NRVOCandidate) {
+      InitializedEntity Entity =
+          InitializedEntity::InitializeResult(Loc, E->getType(), NRVOCandidate);
+      ExprResult MoveResult = this->PerformMoveOrCopyInitialization(
+          Entity, NRVOCandidate, E->getType(), E);
+      if (MoveResult.get())
+        E = MoveResult.get();
+    }
+  }
+
   // FIXME: If the operand is a reference to a variable that's about to go out
   // of scope, we should treat the operand as an xvalue for this overload
   // resolution.
   VarDecl *Promise = FSI->CoroutinePromise;
   ExprResult PC;
   if (E && (isa<InitListExpr>(E) || !E->getType()->isVoidType())) {
     PC = buildPromiseCall(*this, Promise, Loc, "return_value", E);
   } else {
     E = MakeFullDiscardedValueExpr(E).get();
     PC = buildPromiseCall(*this, Promise, Loc, "return_void", None);
   }

cfe/trunk/test/SemaCXX/coroutine-rvo.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+
+namespace std::experimental {
+template <class Promise = void> struct coroutine_handle {
+  coroutine_handle() = default;
+  static coroutine_handle from_address(void *) noexcept;
+};
+
+template <> struct coroutine_handle<void> {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template <class PromiseType>
+  coroutine_handle(coroutine_handle<PromiseType>) noexcept;
+};
+
+template <class... Args>
+struct void_t_imp {
+  using type = void;
+};
+template <class... Args>
+using void_t = typename void_t_imp<Args...>::type;
+
+template <class T, class = void>
+struct traits_sfinae_base {};
+
+template <class T>
+struct traits_sfinae_base<T, void_t<typename T::promise_type>> {
+  using promise_type = typename T::promise_type;
+};
+
+template <class Ret, class... Args>
+struct coroutine_traits : public traits_sfinae_base<Ret> {};
+}
+
+struct suspend_never {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct MoveOnly {
+  MoveOnly() {};
+  MoveOnly(const MoveOnly&) = delete;
+  MoveOnly(MoveOnly&&) noexcept {};
+  ~MoveOnly() {};
+};
+
+template <typename T>
+struct task {
+  struct promise_type {
+    auto initial_suspend() { return suspend_never{}; }
+    auto final_suspend() { return suspend_never{}; }
+    auto get_return_object() { return task{}; }
+    static void unhandled_exception() {}
+    void return_value(T&& value) {}
+  };
+};
+
+task<MoveOnly> f() {
+  MoveOnly value;
+  co_return value;
+}
+
+int main() {
+  f();
+  return 0;
+}
+
+// expected-no-diagnostics