[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a subscriber: lewissbaker.
Quuxplusone added a comment.

One more test to add:

  struct Widget {
      task<Widget> foo() && {
          co_return *this;  // IIUC this should call return_value(Widget&), not return_value(Widget&&)
      }
  };



================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:869
   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();
-    }
+    VarDecl *NRVOCandidate =
+        getCopyElisionCandidate(E->getType(), E, CES_Default);
----------------
aaronpuchert wrote:
> Quuxplusone wrote:
> > aaronpuchert wrote:
> > > Should be renamed to `RVOCandidate`, I don't think NRVO can happen here.
> > (Btw, I have no comment on the actual code change in this patch. I'm here in my capacity as [RVO-explainer](https://www.youtube.com/watch?v=hA1WNtNyNbo)-and-[P1155](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1155r2.html)-author, not code-understander. ;))
> > 
> > What's happening here is never technically "RVO" at all, because there is no "RV". However, the "N" is accurate. (See [my acronym glossary](https://quuxplusone.github.io/blog/2019/08/02/the-tough-guide-to-cpp-acronyms/#rvo-nrvo-urvo) for details.)
> > The important thing here is that when you say `co_return x;` the `x` is //named//, and it //would be// a candidate for NRVO if we were in a situation where NRVO was possible at all.
> > 
> > The actual optimization that is happening here is "implicit move."
> > 
> > I would strongly prefer to keep the name `NRVOCandidate` here, because that is the name that is used for the exact same purpose — computing "implicit move" candidates — in `BuildReturnStmt`. Ideally this code would be factored out so that it appeared in only one place; but until then, gratuitous differences between the two sites should be minimized IMO.
> Hmm, you're completely right. What do you think about `ImplicitMoveCandidate`? Otherwise I'll stick with the current name, as you suggested.
> 
> > Ideally this code would be factored out so that it appeared in only one place; but until then, gratuitous differences between the two sites should be minimized IMO.
> 
> Isn't it already factored out? I let `getCopyElisionCandidate` do all the heavy lifting. (Except filtering out lvalue references...)
> What do you think about `ImplicitMoveCandidate`?

I think that would be more correct in this case, but it wouldn't be parallel with the one in `BuildReturnStmt`, and I'm not sure whether it would be correct to change it over there too.

> Isn't it already factored out?

I see some parallelism in the two other places that call `getCopyElisionCandidate` followed by `PerformMoveOrCopyInitialization`. Maybe this is as factored-out as it can get, but it still looks remarkably parallel. (And I wish `NRVOVariable` was named `NRVOCandidate` in the latter case.)

In `BuildReturnStmt`:

    if (RetValExp)
      NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
    if (!HasDependentReturnType && !RetValExp->isTypeDependent()) {
      // we have a non-void function with an expression, continue checking
      InitializedEntity Entity = InitializedEntity::InitializeResult(ReturnLoc,
                                                                     RetType,
                                                      NRVOCandidate != nullptr);
      ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRVOCandidate,
                                                       RetType, RetValExp);

In `BuildCXXThrow`:

    const VarDecl *NRVOVariable = nullptr;
    if (IsThrownVarInScope)
      NRVOVariable = getCopyElisionCandidate(QualType(), Ex, CES_Strict);

    InitializedEntity Entity = InitializedEntity::InitializeException(
        OpLoc, ExceptionObjectTy,
        /*NRVO=*/NRVOVariable != nullptr);
    ExprResult Res = PerformMoveOrCopyInitialization(
        Entity, NRVOVariable, QualType(), Ex, IsThrownVarInScope);

Naming-wise, I also offer that David Stone's [P1825](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html) introduces the name "implicitly movable entity" for these things, and //maybe// we should call this variable `ImplicitlyMovableEntity`; however, I am not yet sure.


================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:71
+task<MoveOnly> param2val(MoveOnly value) {
+  co_return value;
 }
----------------
aaronpuchert wrote:
> Quuxplusone wrote:
> > This should work equally well with `NoCopyNoMove`, right? It should just call `task<NCNM>::return_value(NCNM&&)`. I don't think you need `MoveOnly` in this test file anymore.
> I thought so, too, but there is some code that probably constructs the coroutine and that needs a move constructor. If you look at the AST:
> 
> ```
> FunctionDecl 0xee2e08 <line:70:1, line:72:1> line:70:16 param2val 'task<MoveOnly> (MoveOnly)'
> |-ParmVarDecl 0xee2cf0 <col:26, col:35> col:35 used value 'MoveOnly'
> `-CoroutineBodyStmt 0xee7df8 <col:42, line:72:1>
>   |-CompoundStmt 0xee71b8 <line:70:42, line:72:1>
>   | `-CoreturnStmt 0xee7190 <line:71:3, col:13>
>   |   |-ImplicitCastExpr 0xee7100 <col:13> 'MoveOnly' xvalue <NoOp>
>   |   | `-DeclRefExpr 0xee3088 <col:13> 'MoveOnly' lvalue ParmVar 0xee2cf0 'value' 'MoveOnly'
>   |   `-CXXMemberCallExpr 0xee7168 <col:3> 'void'
>   |     |-MemberExpr 0xee7138 <col:3> '<bound member function type>' .return_value 0xee5408
>   |     | `-DeclRefExpr 0xee7118 <col:3> 'std::experimental::traits_sfinae_base<task<MoveOnly>, void>::promise_type':'task<MoveOnly>::promise_type' lvalue Var 0xee54e8 '__promise' 'std::experimental::traits_sfinae_base<task<MoveOnly>, void>::promise_type':'task<MoveOnly>::promise_type'
>   |     `-ImplicitCastExpr 0xee7100 <col:13> 'MoveOnly' xvalue <NoOp>
>   |       `-DeclRefExpr 0xee3088 <col:13> 'MoveOnly' lvalue ParmVar 0xee2cf0 'value' 'MoveOnly'
> ```
> 
> So no move constructor here. But then comes a bunch of other stuff, and finally,
> 
> ```
> `-CoroutineBodyStmt 0xee7df8 <col:42, line:72:1>
>   [...]
>   `-DeclStmt 0xee31f0 <line:71:3>
>     `-VarDecl 0xee3118 <col:3> col:3 implicit used value 'MoveOnly' listinit
>       `-CXXConstructExpr 0xee31c0 <col:3> 'MoveOnly' 'void (MoveOnly &&) noexcept'
>         `-CXXStaticCastExpr 0xee30d8 <col:3> 'MoveOnly' xvalue static_cast<struct MoveOnly &&> <NoOp>
>           `-DeclRefExpr 0xee30a8 <col:3> 'MoveOnly' lvalue ParmVar 0xee2cf0 'value' 'MoveOnly'
> ```
Oh, I see! That's initially surprising, but I see why it has to be that way. The parameter comes in to this function on the stack, but the parameter variable has to be part of the coroutine frame, which is heap-allocated. So the first thing this function does is _move_ the parameter from the stack into the heap-allocated frame. Local variables can be constructed right there in the frame, but parameters need to be moved.

I wonder if this quirk is reflected in the CD wording for Coroutines. By my reading, the current CD says "no" — it expects the parameters to get _copied_ (not moved) from the stack into the coroutine frame. Attn @GorNishanov, @lewissbaker. http://eel.is/c++draft/dcl.fct.def.coroutine#5.6


================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:74
 
-// expected-no-diagnostics
+task<Default> lvalue2val(Default& value) {
+  co_return value; // expected-error{{rvalue reference to type 'Default' cannot bind to lvalue of type 'Default'}}
----------------
aaronpuchert wrote:
> Quuxplusone wrote:
> > Ditto here, could you use `NoCopyNoMove` instead of `Default`?
> I used `Default` to show that there is an error even if both copy and move constructor are available, because `return_value` takes a `Default&&` then, but we pass a `Default&` (which is not casted to xvalue).
Okay, that is a reasonable explanation. Me personally, I think overload resolution is so complicated that you have not necessarily made the test "stronger," just "different" — the fact that it passes for `Default` does not at all reassure me that it would necessarily pass for `NCNM` as well. But as YMMV, I won't press further.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68845/new/

https://reviews.llvm.org/D68845