This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tks2103 on Sep 6 2018, 11:05 AM.

Details

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.

Diff Detail

Event Timeline

tks2103 created this revision.Sep 6 2018, 11:05 AM
tks2103 edited the summary of this revision. (Show Details)Sep 6 2018, 11:06 AM
This comment was removed by tks2103.
This comment was removed by tks2103.
tks2103 updated this revision to Diff 164265.Sep 6 2018, 12:07 PM

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

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

modocache accepted this revision.Sep 26 2018, 8:19 AM

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

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.

This revision is now accepted and ready to land.Sep 26 2018, 8:19 AM
GorNishanov accepted this revision.Oct 1 2018, 4:20 PM

LGTM! Thank you for doing this.

tks2103 updated this revision to Diff 168380.Oct 4 2018, 2:30 PM

comply with clang-format

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

This revision was automatically updated to reflect the committed changes.

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 ↗(On Diff #168604)

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

849 ↗(On Diff #168604)

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?

Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 10:07 AM

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 ↗(On Diff #168604)

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 ↗(On Diff #168604)

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

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&.

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 ↗(On Diff #168604)

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 ↗(On Diff #168604)

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

Quuxplusone added inline comments.Oct 9 2019, 2:07 PM
cfe/trunk/lib/Sema/SemaCoroutine.cpp
857–859 ↗(On Diff #168604)

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 added a comment.EditedOct 10 2019, 2:31 PM

@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.

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