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

[Aaron Puchert via Phabricator via llvm-commits]

aaronpuchert added a comment.

@Quuxplusone Thanks for your very helpful comments!

> In which case, this patch (D51741 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 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`, bceause `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 rvalue reference initialization or implicit cast if we're returning a DeclRefExpr referencing a variable that we can consume.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D51741