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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:896
+  if (S.CanPerformCopyInitialization(Entity, ))
+return true;
+} else if (auto *FTD = dyn_cast(D)) {

aaronpuchert wrote:
> Quuxplusone wrote:
> > aaronpuchert wrote:
> > > Overlad resolution can actually still fail if there is a viable 
> > > candidate, namely when there are multiple candidates and none is better 
> > > than all others. It's a bit weird though to fall back to lvalue parameter 
> > > then as if nothing happened.
> > That is an interesting point! I had not considered it during 
> > [P1155](https://wg21.link/p1155r2). I imagine that it might make 
> > implementation of [P1155](https://wg21.link/p1155r2)'s new logic more 
> > difficult.
> > 
> > GCC 8 (but not trunk) implements the behavior I would expect to see for 
> > derived-to-base conversions: https://godbolt.org/z/fj_lNw
> > 
> > C++ always treats "an embarrassment of riches" equivalently to a "famine"; 
> > overload resolution //can// fail due to ambiguity just as easily as it can 
> > fail due to no candidates at all. I agree it's "a bit weird," but it would 
> > be vastly weirder if C++ did anything //different// from its usual behavior 
> > in this case.
> > 
> > I'm still amenable to the idea that `co_return` should simply not do the 
> > copy-elision or implicit-move optimizations at all. I wish I knew some 
> > use-cases for `co_return`, so that we could see if the optimization is 
> > useful to anyone.
> > I agree it's "a bit weird," but it would be vastly weirder if C++ did 
> > anything different from its usual behavior in this case.
> 
> I would find it more natural to throw an error, i.e. not do the fallback, in 
> the case of ambiguity. So the fallback should in my opinion only happen when 
> there are no viable overload candidates, not when there are too many.
> 
> I see your construction as follows: we add both operations that take an 
> lvalue and those that take an rvalue to a bigger “overload set”, and order 
> those that take rvalues as higher/better than those that don't. One could say 
> that we do overload resolution on a `T&&` argument where we allow binding a 
> `T&&` to a `T&`, but this is less preferable in the overload ordering.
In light of your comment

>>! In D88220#2435959, @Quuxplusone wrote:
> This example will be mentioned in my upcoming (not-yet-finished) WG21 paper 
> [P2266](https://wg21.link/p2266), as an example of why the two-pass mechanism 
> sucks and should be removed from C++2b.

could we re-evaluate this point? I'm not sure what your paper is going to say, 
but I think I agree that the two-pass mechanism is weird.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 305353.
aaronpuchert added a comment.
Herald added a subscriber: lxfind.

Rebase and add a comment about the limitations of this implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-rvo.cpp

Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -39,10 +39,14 @@
 };
 
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly &&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove &) = delete;
 };
 
 template 
@@ -52,18 +56,93 @@
 auto final_suspend() noexcept { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T &) {} // expected-note 2{{passing argument}}
   };
 };
 
-task f() {
-  MoveOnly value;
+task local2val() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+task local2ref() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+// We need the move constructor for construction of the coroutine.
+task param2val(MoveOnly value) {
+  co_return value;
+}
+
+task lvalue2val(NoCopyNoMove ) {
+  co_return value; // expected-error{{rvalue reference to type 'NoCopyNoMove' cannot bind to lvalue of type 'NoCopyNoMove'}}
+}
+
+task rvalue2val(NoCopyNoMove &) {
+  co_return value;
+}
+
+task lvalue2ref(NoCopyNoMove ) {
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+task rvalue2ref(NoCopyNoMove &) {
+  co_return value;
+}
+
+struct To {
+  operator MoveOnly() &&;
+};
+task conversion_operator() {
+  To t;
+  co_return t;
+}
+
+struct Construct {
+  Construct(MoveOnly);
+};
+task converting_constructor() {
+  MoveOnly w;
+  co_return w;
+}
+
+struct Derived : MoveOnly {};
+task derived2base() {
+  Derived result;
+  co_return result;
+}
+
+struct RetThis {
+  task foo() && {
+co_return *this; // expected-error{{rvalue reference to type 'RetThis' cannot bind to lvalue of type 'RetThis'}}
+  }
+};
+
+template 
+struct is_same { static constexpr bool value = false; };
+
+template 
+struct is_same { static constexpr bool value = true; };
+
+template 
+struct template_return_task {
+  struct promise_type {
+auto initial_suspend() { return suspend_never{}; }
+auto final_suspend() noexcept { return suspend_never{}; }
+auto get_return_object() { return template_return_task{}; }
+static void unhandled_exception();
+template 
+void return_value(U &) {
+  static_assert(is_same::value);
+}
+  };
+};
+
+template_return_task param2template(MoveOnly value) {
+  co_return value; // We should deduce U = MoveOnly.
 }
 
-// expected-no-diagnostics
+template_return_task lvalue2template(NoCopyNoMove ) {
+  co_return value; // We should deduce U = NoCopyNoMove&.
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -954,6 +954,82 @@
   return BuildCoreturnStmt(Loc, E);
 }
 
+// [class.copy.elision]p3: "If the expression in a [...] co_­return statement
+// is a (possibly parenthesized) id-expression that names an implicitly
+// movable entity declared in the body or parameter-declaration-clause of
+// the innermost enclosing function or lambda-expression, [...] overload
+// resolution to select [...] the return_­value overload to call is first
+// performed as if the expression or operand were an rvalue. If the first
+// overload resolution fails or was not performed, overload resolution is
+// performed again, considering the expression or operand as an lvalue."
+//
+// Our implementation deviates from the standard in two ways:
+// 1) We only consider cases where return_value is a function or function
+//template, so we never try implicitly moving in the case of a function
+//pointer or object of class type with operator() overload.
+// 2) We don't fall back if overload resolution failed due to ambiguity, only
+//if no candidate works. One could say that we do overload resolution on
+//both (const) T& and T&& while treating candidates for T&& as better fit:
+//so no candidate for T&& falls back to T&, but multiple equivalent
+//candidates are an error.
+static bool shouldMoveReturnExpr(Sema , RecordDecl 

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:951
 
   Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsImplicit);
   return Res;

Another interesting question is whether this `E` should include the xvalue 
cast, if we did it. I have no idea what would be expected here. I'm tending 
towards replacing `E` by `Arg`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Not answering inline because comments are getting longer.

> I still think that it makes sense to do implicit-move on any control-flow 
> construct that "exits" the current function. Right now that's `return` and 
> `throw`, and they both do implicit-move (albeit inconsistently with plenty of 
> implementation divergence documented in P1155 ). 
> C++2a adds `co_return` as another way to "exit." I think it would be 
> un-graceful and inconsistent to permit `return p` but require `co_return 
> std::move(p)`.

Even with `return` it's not obvious where to draw the line: basically we still 
require that nothing non-trivial happens between the `ReturnStmt` and the 
`DeclRefExpr`, so for example `return f(x)` won't be turned into `return 
f(std::move(x))`, and `return a + b` won't be turned into `return std::move(a) 
+ std::move(b)`. It's clear why this can't always happen: if an lvalue is 
referenced more than once, we might move it more than once. But one could argue 
that we should still move all local variables that only occur once. I work in a 
code base with a lot of code like

  SomeBuilder b;
  b.doStuff();
  return std::move(b).getResult();

calling a `getResult() &&` method at the end. It would be nice if I could omit 
the move. But I see why that's hard.

So basically we have the cases where implicit move is more or less necessary 
(because explicit moving would prevent NRVO), and we have the cases where it 
would be possible. And the difficulty is to find the right cutoff. If I 
understand this correctly, the current rules basically say that if we directly 
return a `DeclRefExpr`, possibly with implicit conversions, that will be 
implicitly moved, if possible. (Although it's a bit weird that we silently fall 
back in the case of ambiguity.)

With `co_return` the implicit move is never “necessary”, because NRVO can't 
happen, so we could just never do it. You're right that could be considered an 
inconsistency, but it wouldn't be hard to remember: "for `co_return`, always 
move."

> You've noticed that to codegen `t.return_value(x)` requires not just overload 
> resolution, but actually figuring out whether `task::return_value` is a 
> function at all —

Function templates definitely make sense to me, I also added test cases for 
that.

> it might be a static data member of function-pointer type, or something even 
> wilder 
> .

It could also be an object of class type with an `operator()`. I'm actually not 
sure if the standard intentionally allows all these variants.

(Unrelated, but in that blog post you write “for backward compatibility, C++17 
had to make `noexcept` function pointers implicitly convertible to 
non-`noexcept` function pointers.” I don't see this as a matter of backward 
compatibility: just like you can implicitly cast a reference/pointer to 
`const`, you should be able to cast `noexcept` away from pointers/references.)

> But eliminating implicit-move won't make that complexity go away, will it? 
> Doing implicit-move just means doing the required overload resolution twice 
> instead of once. That should be easy, compared to the rest of the mess.

Initially I thought so, and I wrote some very elegant (yet unfortunately wrong) 
code. I called `buildPromiseCall`, which returns an `ExprResult`, and if that 
was invalid I tried again without the xvalue conversion. Unfortunately, if that 
overload resolution fails, I get an error message //even if I discard the 
resulting expression//, and compilation fails. (I would also get warnings, if 
there are any.) So I looked into the call stack if I could disentangle the 
overload resolution from the diagnostics, but that wasn't so easy. That's why 
this is so much code.

At first I thought this was weird and there should be functionality for this in 
`Sema`, but I think there isn't. I believe that's because there is basically no 
precedent to “do overload resolution, and if that fails, silently do something 
else instead”. Normally, if overload resolution fails, that's just an immediate 
error, no need to wait for anything else. So basically I need to simulate 
`Sema::BuildCallExpr`, but without throwing errors. Fortunately many code paths 
aren't needed, so it's not terribly complicated, but I'm still missing at least 
two cases (function pointers and function objects).

For `return` this is much simpler, because we just do an initialization, and 
there are ways to “try” initialization without immediately throwing an error. 
For `co_return` I need to try (parameter) initialization **and** try to resolve 
the callee.

> That said, I would be happy to start a thread among you, me, David Stone, and 
> the EWG mailing list to consider removing the words "or `co_return`" from 
> class.copy.elision/3 . Say the 
> word.

I'm somewhat undecided. Things would perhaps also be 

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

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

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:896
+  if (S.CanPerformCopyInitialization(Entity, ))
+return true;
+} else if (auto *FTD = dyn_cast(D)) {

aaronpuchert wrote:
> Overlad resolution can actually still fail if there is a viable candidate, 
> namely when there are multiple candidates and none is better than all others. 
> It's a bit weird though to fall back to lvalue parameter then as if nothing 
> happened.
That is an interesting point! I had not considered it during 
[P1155](https://wg21.link/p1155r2). I imagine that it might make implementation 
of [P1155](https://wg21.link/p1155r2)'s new logic more difficult.

GCC 8 (but not trunk) implements the behavior I would expect to see for 
derived-to-base conversions: https://godbolt.org/z/fj_lNw

C++ always treats "an embarrassment of riches" equivalently to a "famine"; 
overload resolution //can// fail due to ambiguity just as easily as it can fail 
due to no candidates at all. I agree it's "a bit weird," but it would be vastly 
weirder if C++ did anything //different// from its usual behavior in this case.

I'm still amenable to the idea that `co_return` should simply not do the 
copy-elision or implicit-move optimizations at all. I wish I knew some 
use-cases for `co_return`, so that we could see if the optimization is useful 
to anyone.



Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:65
+  NoCopyNoMove value;
+  co_return value;
+}

@aaronpuchert wrote:
> Given the complexities of this implementation, I'm beginning to doubt whether 
> implicit moves make sense for `co_return` at all. Since there can never be 
> any kind of RVO, why not always require an explicit `std::move`? Implicit 
> moves on return were introduced because an explicit move would inhibit NRVO, 
> and without move there wouldn't be a graceful fallback for implementations 
> that don't have NRVO.

I still think that it makes sense to do implicit-move on //any// control-flow 
construct that "exits" the current function. Right now that's `return` and 
`throw`, and they both do implicit-move (albeit inconsistently with plenty of 
implementation divergence documented in [P1155](http://wg21.link/p1155r2)). 
C++2a adds `co_return` as another way to "exit." I think it would be 
un-graceful and inconsistent to permit `return p` but require `co_return 
std::move(p)`.

Now, I do think that C++2a Coroutines are needlessly complicated, which makes 
the implementation needlessly complicated. You've noticed that to codegen 
`t.return_value(x)` requires not just overload resolution, but actually 
figuring out whether `task::return_value` is a function at all — it might be a 
static data member of function-pointer type, or something [even 
wilder](https://quuxplusone.github.io/blog/2019/09/18/cppcon-whiteboard-puzzle/).
 But eliminating implicit-move won't make that complexity go away, will it? 
Doing implicit-move just means doing the required overload resolution twice 
instead of once. That //should// be easy, compared to the rest of the mess.

That said, I would be happy to start a thread among you, me, David Stone, and 
the EWG mailing list to consider removing the words "or `co_return`" from 
[class.copy.elision/3](http://eel.is/c++draft/class.copy.elision#3.1). Say the 
word.

[EDIT: aw shoot, I never submitted this comment last time]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 226980.
aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

Add test case where check for non-deduced parameter conversions is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-rvo.cpp

Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -39,10 +39,14 @@
 };
 
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly &&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove &) = delete;
 };
 
 template 
@@ -52,18 +56,111 @@
 auto final_suspend() { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T &) {} // expected-note 2{{passing argument}}
+  };
+};
+
+task local2val() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+task local2ref() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+// We need the move constructor for construction of the coroutine.
+task param2val(MoveOnly value) {
+  co_return value;
+}
+
+task lvalue2val(NoCopyNoMove ) {
+  co_return value; // expected-error{{rvalue reference to type 'NoCopyNoMove' cannot bind to lvalue of type 'NoCopyNoMove'}}
+}
+
+task rvalue2val(NoCopyNoMove &) {
+  co_return value;
+}
+
+task lvalue2ref(NoCopyNoMove ) {
+  co_return value;
+}
+
+task rvalue2ref(NoCopyNoMove &) {
+  co_return value;
+}
+
+struct To {
+  operator MoveOnly() &&;
+};
+task conversion_operator() {
+  To t;
+  co_return t;
+}
+
+struct Construct {
+  Construct(MoveOnly);
+};
+task converting_constructor() {
+  MoveOnly w;
+  co_return w;
+}
+
+struct Derived : MoveOnly {};
+task derived2base() {
+  Derived result;
+  co_return result;
+}
+
+struct RetThis {
+  task foo() && {
+co_return *this; // expected-error{{rvalue reference to type 'RetThis' cannot bind to lvalue of type 'RetThis'}}
+  }
+};
+
+template 
+struct is_same { static constexpr bool value = false; };
+template 
+struct is_same { static constexpr bool value = true; };
+
+template 
+struct enable_if;
+template <>
+struct enable_if { };
+template <>
+struct enable_if { struct type; };
+
+struct MoveOnly2 : MoveOnly {};
+
+template 
+struct template_return_task {
+  struct promise_type {
+auto initial_suspend() { return suspend_never{}; }
+auto final_suspend() { return suspend_never{}; }
+auto get_return_object() { return template_return_task{}; }
+static void unhandled_exception();
+template ::value>::type>
+void return_value(U &) {
+  static_assert(is_same::value);
+}
+template 
+void return_value(int value) {}
   };
 };
 
-task f() {
-  MoveOnly value;
+// We should deduce U = MoveOnly via implicit move.
+template_return_task param2template(MoveOnly value) {
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+// We should deduce U = NoCopyNoMove&, because we can't implicitly move.
+template_return_task lvalue2template(NoCopyNoMove ) {
+  co_return value;
 }
 
-// expected-no-diagnostics
+// We should deduce U = MoveOnly2&, because one overload is a substitution
+// failure and for the other implicit conversions fail.
+template_return_task param2template(MoveOnly2 value) {
+  co_return value;
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -851,6 +851,72 @@
   return BuildCoreturnStmt(Loc, E);
 }
 
+// [class.copy.elision]p3: If the expression in a [...] co_­return statement
+// is a (possibly parenthesized) id-expression that names an implicitly
+// movable entity declared in the body or parameter-declaration-clause of
+// the innermost enclosing function or lambda-expression, [...] overload
+// resolution to select [...] the return_­value overload to call is first
+// performed as if the expression or operand were an rvalue. If the first
+// overload resolution fails or was not performed, overload resolution is
+// performed again, considering the expression or operand as an lvalue.
+static bool shouldMoveReturnExpr(Sema , RecordDecl *PromiseTypeDecl,
+ Expr *E) {
+  // Check if we're allowed to implicitly move.
+  VarDecl *ImplicitMoveCandidate =
+  

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:896
+  if (S.CanPerformCopyInitialization(Entity, ))
+return true;
+} else if (auto *FTD = dyn_cast(D)) {

Overlad resolution can actually still fail if there is a viable candidate, 
namely when there are multiple candidates and none is better than all others. 
It's a bit weird though to fall back to lvalue parameter then as if nothing 
happened.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:903-909
+QualType ParamType = ParamTypes[0];
+if (!ParamType->isDependentType()) {
+  InitializedEntity Entity = 
InitializedEntity::InitializeParameter(
+  S.Context, ParamType, false);
+  if (!S.CanPerformCopyInitialization(Entity, ))
+return true;
+}

I should add a test case where this is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked 15 inline comments as done.
aaronpuchert added a comment.

Given the complexities of this implementation, I'm beginning to doubt whether 
implicit moves make sense for `co_return` at all. Since there can never be any 
kind of RVO, why not always require an explicit `std::move`? Implicit moves on 
`return` were introduced because an explicit move would inhibit NRVO, and 
without move there wouldn't be a graceful fallback for implementations that 
don't have NRVO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 225149.
aaronpuchert added a comment.

Apply clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-rvo.cpp

Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -39,10 +39,14 @@
 };
 
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly &&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove &) = delete;
 };
 
 template 
@@ -52,18 +56,93 @@
 auto final_suspend() { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T &) {} // expected-note 2{{passing argument}}
   };
 };
 
-task f() {
-  MoveOnly value;
+task local2val() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+task local2ref() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+// We need the move constructor for construction of the coroutine.
+task param2val(MoveOnly value) {
+  co_return value;
+}
+
+task lvalue2val(NoCopyNoMove ) {
+  co_return value; // expected-error{{rvalue reference to type 'NoCopyNoMove' cannot bind to lvalue of type 'NoCopyNoMove'}}
+}
+
+task rvalue2val(NoCopyNoMove &) {
+  co_return value;
+}
+
+task lvalue2ref(NoCopyNoMove ) {
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+task rvalue2ref(NoCopyNoMove &) {
+  co_return value;
+}
+
+struct To {
+  operator MoveOnly() &&;
+};
+task conversion_operator() {
+  To t;
+  co_return t;
+}
+
+struct Construct {
+  Construct(MoveOnly);
+};
+task converting_constructor() {
+  MoveOnly w;
+  co_return w;
+}
+
+struct Derived : MoveOnly {};
+task derived2base() {
+  Derived result;
+  co_return result;
+}
+
+struct RetThis {
+  task foo() && {
+co_return *this; // expected-error{{rvalue reference to type 'RetThis' cannot bind to lvalue of type 'RetThis'}}
+  }
+};
+
+template 
+struct is_same { static constexpr bool value = false; };
+
+template 
+struct is_same { static constexpr bool value = true; };
+
+template 
+struct template_return_task {
+  struct promise_type {
+auto initial_suspend() { return suspend_never{}; }
+auto final_suspend() { return suspend_never{}; }
+auto get_return_object() { return template_return_task{}; }
+static void unhandled_exception();
+template 
+void return_value(U &) {
+  static_assert(is_same::value);
+}
+  };
+};
+
+template_return_task param2template(MoveOnly value) {
+  co_return value; // We should deduce U = MoveOnly.
 }
 
-// expected-no-diagnostics
+template_return_task lvalue2template(NoCopyNoMove ) {
+  co_return value; // We should deduce U = NoCopyNoMove&.
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -851,6 +851,72 @@
   return BuildCoreturnStmt(Loc, E);
 }
 
+// [class.copy.elision]p3: If the expression in a [...] co_­return statement
+// is a (possibly parenthesized) id-expression that names an implicitly
+// movable entity declared in the body or parameter-declaration-clause of
+// the innermost enclosing function or lambda-expression, [...] overload
+// resolution to select [...] the return_­value overload to call is first
+// performed as if the expression or operand were an rvalue. If the first
+// overload resolution fails or was not performed, overload resolution is
+// performed again, considering the expression or operand as an lvalue.
+static bool shouldMoveReturnExpr(Sema , RecordDecl *PromiseTypeDecl,
+ Expr *E) {
+  // Check if we're allowed to implicitly move.
+  VarDecl *ImplicitMoveCandidate =
+  S.getCopyElisionCandidate(E->getType(), E, Sema::CES_Default);
+  if (!ImplicitMoveCandidate)
+return false;
+  if (ImplicitMoveCandidate->getType()->isLValueReferenceType())
+return false;
+
+  ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, E->getType(), CK_NoOp, E,
+VK_XValue);
+
+  // Lookup return_value methods in the promise type, and check if any accepts
+  // the moved expression. If it does, we do the move.
+  DeclarationNameInfo NameInfo(().get("return_value"),
+   SourceLocation{});
+  LookupResult LR(S, 

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 225147.
aaronpuchert added a comment.

Add tests suggested by @Quuxplusone and add fallback to call by lvalue 
reference.

The latter turned out more complicated than I thought, because there seems to 
be no easy way to just try building a call expression without emitting errors 
if anything goes wrong. So essentially I look up the name myself and check if 
someone takes an rvalue reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-rvo.cpp

Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -39,10 +39,14 @@
 };
 
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly&&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove&) = delete;
 };
 
 template 
@@ -52,18 +56,89 @@
 auto final_suspend() { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T&& value) {} // expected-note 2{{passing argument}}
   };
 };
 
-task f() {
-  MoveOnly value;
+task local2val() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+task local2ref() {
+  NoCopyNoMove value;
+  co_return value;
+}
+
+// We need the move constructor for construction of the coroutine.
+task param2val(MoveOnly value) {
+  co_return value;
+}
+
+task lvalue2val(NoCopyNoMove& value) {
+  co_return value; // expected-error{{rvalue reference to type 'NoCopyNoMove' cannot bind to lvalue of type 'NoCopyNoMove'}}
+}
+
+task rvalue2val(NoCopyNoMove&& value) {
+  co_return value;
+}
+
+task lvalue2ref(NoCopyNoMove& value) {
+  co_return value;
+}
+
+task rvalue2ref(NoCopyNoMove&& value) {
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+struct To { operator MoveOnly() &&; };
+task conversion_operator() {
+  To t;
+  co_return t;
 }
 
-// expected-no-diagnostics
+struct Construct { Construct(MoveOnly); };
+task converting_constructor() {
+  MoveOnly w;
+  co_return w;
+}
+
+struct Derived : MoveOnly {};
+task derived2base() {
+  Derived result;
+  co_return result;
+}
+
+struct RetThis {
+  task foo() && {
+co_return *this; // expected-error{{rvalue reference to type 'RetThis' cannot bind to lvalue of type 'RetThis'}}
+  }
+};
+
+template 
+struct is_same { static constexpr bool value = false; };
+
+template 
+struct is_same { static constexpr bool value = true; };
+
+template 
+struct template_return_task {
+  struct promise_type {
+auto initial_suspend() { return suspend_never{}; }
+auto final_suspend() { return suspend_never{}; }
+auto get_return_object() { return template_return_task{}; }
+static void unhandled_exception();
+template 
+void return_value(U &) {
+  static_assert(is_same::value);
+}
+  };
+};
+
+template_return_task param2template(MoveOnly value) {
+  co_return value; // We should deduce U = MoveOnly.
+}
+
+template_return_task lvalue2template(NoCopyNoMove ) {
+  co_return value; // We should deduce U = NoCopyNoMove&.
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -851,6 +851,71 @@
   return BuildCoreturnStmt(Loc, E);
 }
 
+// [class.copy.elision]p3: If the expression in a [...] co_­return statement
+// is a (possibly parenthesized) id-expression that names an implicitly
+// movable entity declared in the body or parameter-declaration-clause of
+// the innermost enclosing function or lambda-expression, [...] overload
+// resolution to select [...] the return_­value overload to call is first
+// performed as if the expression or operand were an rvalue. If the first
+// overload resolution fails or was not performed, overload resolution is
+// performed again, considering the expression or operand as an lvalue.
+static bool shouldMoveReturnExpr(Sema& S, RecordDecl *PromiseTypeDecl, Expr* E)
+{
+  // Check if we're allowed to implicitly move.
+  VarDecl *ImplicitMoveCandidate =
+S.getCopyElisionCandidate(E->getType(), E, Sema::CES_Default);
+  if (!ImplicitMoveCandidate)
+return false;
+  if (ImplicitMoveCandidate->getType()->isLValueReferenceType())
+return false;
+
+  ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, 

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

@GorNishanov Do I read the standard correctly that there are no constraints on 
what `return_value` is? That is, could it also be a function pointer or 
function object?

  struct promise_function_pointer {
// ...
void (*return_value)(T&& value);
  };
  struct promise_function_object {
// ...
struct { void operator()(T&& value) {}; } return_value;
  };

In both cases, the expression `.return_value()` 
would be well-formed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

I'll add your test case, but I'll probably reuse the existing data structures.

In D68845#1705430 , @Quuxplusone wrote:

> Oh, and can you please make sure there are test cases for all the various 
> cases covered in P1155 
> ? 
> Specifically, I would expect all three of the following test cases to compile 
> successfully. It looks like they compile successfully in trunk right now 
> (Godbolt ), so we're just testing that 
> they don't get broken in the future.
>
>   struct Widget { Widget(); Widget(const Widget&) = delete; Widget(Widget&&); 
> };
>   struct To { operator Widget() &&; };
>   task nine() { To t; co_return t; }
>  
>   struct Fowl { Fowl(Widget); };
>   task eleven() { Widget w; co_return w; }
>  
>   struct Base { Base(); Base(const Base&) = delete; Base(Base&&); };
>   struct Derived : Base {};
>   task thirteen() { Derived result; co_return result; }
>


These seem to work automatically, because in the end we're just building a 
function call, which does the right implicit conversions if needed.

In D68845#1706193 , @Quuxplusone wrote:

> One more test to add:
>
>   struct Widget {
>   task foo() && {
>   co_return *this;  // IIUC this should call return_value(Widget&), 
> not return_value(Widget&&)
>   }
>   };
>


We're currently not considering `*this` as implicitly movable, because it's not 
a DeclRefExpr.




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);

Quuxplusone wrote:
> 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, 

[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 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 param2val(MoveOnly value) {
+  co_return value;
 }

aaronpuchert wrote:
> Quuxplusone wrote:
> > This should work equally well with `NoCopyNoMove`, right? It should just 
> > call `task::return_value(NCNM&&)`. I don't think you need 

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert planned changes to this revision.
aaronpuchert added a comment.

In D51741#1702038 , @Quuxplusone wrote:

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


Still need to address that. With this change we're only doing the first step, 
we still need the fallback.




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);

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



Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:71
+task param2val(MoveOnly value) {
+  co_return value;
 }

Quuxplusone wrote:
> This should work equally well with `NoCopyNoMove`, right? It should just call 
> `task::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:16 param2val 
'task (MoveOnly)'
|-ParmVarDecl 0xee2cf0  col:35 used value 'MoveOnly'
`-CoroutineBodyStmt 0xee7df8 
  |-CompoundStmt 0xee71b8 
  | `-CoreturnStmt 0xee7190 
  |   |-ImplicitCastExpr 0xee7100  'MoveOnly' xvalue 
  |   | `-DeclRefExpr 0xee3088  'MoveOnly' lvalue ParmVar 0xee2cf0 
'value' 'MoveOnly'
  |   `-CXXMemberCallExpr 0xee7168  'void'
  | |-MemberExpr 0xee7138  '' 
.return_value 0xee5408
  | | `-DeclRefExpr 0xee7118  
'std::experimental::traits_sfinae_base, 
void>::promise_type':'task::promise_type' lvalue Var 0xee54e8 
'__promise' 'std::experimental::traits_sfinae_base, 
void>::promise_type':'task::promise_type'
  | `-ImplicitCastExpr 0xee7100  'MoveOnly' xvalue 
  |   `-DeclRefExpr 0xee3088  'MoveOnly' lvalue ParmVar 0xee2cf0 
'value' 'MoveOnly'
```

So no move constructor here. But then comes a bunch of other stuff, and finally,

```
`-CoroutineBodyStmt 0xee7df8 
  [...]
  `-DeclStmt 0xee31f0 
`-VarDecl 0xee3118  col:3 implicit used value 'MoveOnly' listinit
  `-CXXConstructExpr 0xee31c0  'MoveOnly' 'void (MoveOnly &&) 
noexcept'
`-CXXStaticCastExpr 0xee30d8  'MoveOnly' xvalue 
static_cast 
  `-DeclRefExpr 0xee30a8  'MoveOnly' lvalue ParmVar 0xee2cf0 
'value' 'MoveOnly'
```



Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:74
 
-// expected-no-diagnostics
+task lvalue2val(Default& value) {
+  co_return value; // expected-error{{rvalue reference to type 'Default' 
cannot bind to lvalue of type 'Default'}}

Quuxplusone wrote:
> Ditto here, could you use `NoCopyNoMove` instead of `Default`?
I used 

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

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

Quuxplusone added a comment.

Oh, and can you please make sure there are test cases for all the various cases 
covered in P1155 
? 
Specifically, I would expect all three of the following test cases to compile 
successfully. It looks like they compile successfully in trunk right now 
(Godbolt ), so we're just testing that they 
don't get broken in the future.

  struct Widget { Widget(); Widget(const Widget&) = delete; Widget(Widget&&); };
  struct To { operator Widget() &&; };
  task nine() { To t; co_return t; }
  
  struct Fowl { Fowl(Widget); };
  task eleven() { Widget w; co_return w; }
  
  struct Base { Base(); Base(const Base&) = delete; Base(Base&&); };
  struct Derived : Base {};
  task thirteen() { Derived result; co_return result; }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Quuxplusone added inline comments.



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



Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:71
+task param2val(MoveOnly value) {
+  co_return value;
 }

This should work equally well with `NoCopyNoMove`, right? It should just call 
`task::return_value(NCNM&&)`. I don't think you need `MoveOnly` in this 
test file anymore.



Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:74
 
-// expected-no-diagnostics
+task lvalue2val(Default& value) {
+  co_return value; // expected-error{{rvalue reference to type 'Default' 
cannot bind to lvalue of type 'Default'}}

Ditto here, could you use `NoCopyNoMove` instead of `Default`?



Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:86
+
+task rvalue2ref(Default&& value) {
+  co_return value; // expected-error{{non-const lvalue reference to type 
'Default' cannot bind to a temporary of type 'Default'}}

And ditto here: `NoCopyNoMove` instead of `Default`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



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);

Should be renamed to `RVOCandidate`, I don't think NRVO can happen here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: GorNishanov, modocache, Quuxplusone.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
aaronpuchert updated this revision to Diff 224513.
aaronpuchert added a comment.

Also remove FIXME comment.


The implementation of return value optimization in D51741 
 turns

  co_return value;

which is essentially

  __promise.return_value(value);

into

  __promise.return_value(decltype(std::move(value)));

instead of

  __promise.return_value(std::move(value));

Instead of doing a copy/move initialization, which is only required for
regular return statements, we just cast to an xvalue reference when we
can consume an object.

Also simplifies the test: some flags aren't needed, neither is main.
Instead we now check that no move constructor is emitted in certain
cases. (That is, we actually delete the move constructor.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68845

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-rvo.cpp


Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z 
-fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -38,11 +38,17 @@
   void await_resume() noexcept;
 };
 
+struct Default {};
+
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly&&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove&) = delete;
 };
 
 template 
@@ -52,18 +58,31 @@
 auto final_suspend() { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T&& value) {} // expected-note 2{{passing argument}}
   };
 };
 
-task f() {
-  MoveOnly value;
+task local2val() {
+  NoCopyNoMove value;
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+task param2val(MoveOnly value) {
+  co_return value;
 }
 
-// expected-no-diagnostics
+task lvalue2val(Default& value) {
+  co_return value; // expected-error{{rvalue reference to type 'Default' 
cannot bind to lvalue of type 'Default'}}
+}
+
+task rvalue2val(NoCopyNoMove&& value) {
+  co_return value;
+}
+
+task lvalue2ref(NoCopyNoMove& value) {
+  co_return value;
+}
+
+task rvalue2ref(Default&& value) {
+  co_return value; // expected-error{{non-const lvalue reference to type 
'Default' cannot bind to a temporary of type 'Default'}}
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -866,20 +866,13 @@
 
   // 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();
-}
+VarDecl *NRVOCandidate =
+getCopyElisionCandidate(E->getType(), E, CES_Default);
+if (NRVOCandidate && !NRVOCandidate->getType()->isLValueReferenceType())
+  E = ImplicitCastExpr::Create(Context, E->getType(), CK_NoOp, E, {},
+   VK_XValue);
   }
 
-  // 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(E) || !E->getType()->isVoidType())) {


Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -38,11 +38,17 @@
   void await_resume() noexcept;
 };
 
+struct Default {};
+
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly&&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  

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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 224513.
aaronpuchert added a comment.

Also remove FIXME comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutine-rvo.cpp


Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z 
-fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -38,11 +38,17 @@
   void await_resume() noexcept;
 };
 
+struct Default {};
+
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly&&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove&) = delete;
 };
 
 template 
@@ -52,18 +58,31 @@
 auto final_suspend() { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T&& value) {} // expected-note 2{{passing argument}}
   };
 };
 
-task f() {
-  MoveOnly value;
+task local2val() {
+  NoCopyNoMove value;
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+task param2val(MoveOnly value) {
+  co_return value;
 }
 
-// expected-no-diagnostics
+task lvalue2val(Default& value) {
+  co_return value; // expected-error{{rvalue reference to type 'Default' 
cannot bind to lvalue of type 'Default'}}
+}
+
+task rvalue2val(NoCopyNoMove&& value) {
+  co_return value;
+}
+
+task lvalue2ref(NoCopyNoMove& value) {
+  co_return value;
+}
+
+task rvalue2ref(Default&& value) {
+  co_return value; // expected-error{{non-const lvalue reference to type 
'Default' cannot bind to a temporary of type 'Default'}}
+}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -866,20 +866,13 @@
 
   // 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();
-}
+VarDecl *NRVOCandidate =
+getCopyElisionCandidate(E->getType(), E, CES_Default);
+if (NRVOCandidate && !NRVOCandidate->getType()->isLValueReferenceType())
+  E = ImplicitCastExpr::Create(Context, E->getType(), CK_NoOp, E, {},
+   VK_XValue);
   }
 
-  // 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(E) || !E->getType()->isVoidType())) {


Index: clang/test/SemaCXX/coroutine-rvo.cpp
===
--- clang/test/SemaCXX/coroutine-rvo.cpp
+++ clang/test/SemaCXX/coroutine-rvo.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++17 -fcoroutines-ts -fsyntax-only %s
 
 namespace std::experimental {
 template  struct coroutine_handle {
@@ -38,11 +38,17 @@
   void await_resume() noexcept;
 };
 
+struct Default {};
+
 struct MoveOnly {
-  MoveOnly() {};
+  MoveOnly() = default;
   MoveOnly(const MoveOnly&) = delete;
-  MoveOnly(MoveOnly&&) noexcept {};
-  ~MoveOnly() {};
+  MoveOnly(MoveOnly&&) = default;
+};
+
+struct NoCopyNoMove {
+  NoCopyNoMove() = default;
+  NoCopyNoMove(const NoCopyNoMove&) = delete;
 };
 
 template 
@@ -52,18 +58,31 @@
 auto final_suspend() { return suspend_never{}; }
 auto get_return_object() { return task{}; }
 static void unhandled_exception() {}
-void return_value(T&& value) {}
+void return_value(T&& value) {} // expected-note 2{{passing argument}}
   };
 };
 
-task f() {
-  MoveOnly value;
+task local2val() {
+  NoCopyNoMove value;
   co_return value;
 }
 
-int main() {
-  f();
-  return 0;
+task param2val(MoveOnly value) {
+  co_return value;
 }
 
-// expected-no-diagnostics
+task lvalue2val(Default& value) {
+  co_return value; // expected-error{{rvalue reference to type 'Default' cannot bind to lvalue of type 'Default'}}
+}
+
+task rvalue2val(NoCopyNoMove&& value) {