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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
