[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG43f5085fa80f: [Coroutines] Fix premature conversion of 
return object (authored by bruno).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

Files:
  clang/include/clang/AST/StmtCXX.h
  clang/lib/AST/StmtCXX.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CodeGenCoroutines/coro-gro.cpp
  clang/test/CodeGenCoroutines/pr59221.cpp
  clang/test/SemaCXX/coroutine-no-move-ctor.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
@@ -13,8 +13,6 @@
 
 explicit task(promise_type& p) { throw 1; p.return_val = this; }
 
-task(const task&) = delete;
-
 T value;
 };
 
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -934,7 +934,7 @@
 
 extern "C" int f(mismatch_gro_type_tag2) {
   // cxx2b-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
-  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
+  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
Index: clang/test/SemaCXX/coroutine-no-move-ctor.cpp
===
--- clang/test/SemaCXX/coroutine-no-move-ctor.cpp
+++ clang/test/SemaCXX/coroutine-no-move-ctor.cpp
@@ -15,10 +15,13 @@
   };
   using promise_type = invoker_promise;
   invoker() {}
-  invoker(const invoker &) = delete;
-  invoker =(const invoker &) = delete;
-  invoker(invoker &&) = delete;
-  invoker =(invoker &&) = delete;
+  // TODO: implement RVO for get_return_object type matching
+  // function return type.
+  //
+  // invoker(const invoker &) = delete;
+  // invoker =(const invoker &) = delete;
+  // invoker(invoker &&) = delete;
+  // invoker =(invoker &&) = delete;
 };
 
 invoker f() {
Index: clang/test/CodeGenCoroutines/pr59221.cpp
===
--- clang/test/CodeGenCoroutines/pr59221.cpp
+++ clang/test/CodeGenCoroutines/pr59221.cpp
@@ -2,7 +2,7 @@
 //
 // REQUIRES: x86-registered-target
 //
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O1 -S -emit-llvm -o - | FileCheck %s
 
 #include "Inputs/coroutine.h"
 
Index: clang/test/CodeGenCoroutines/coro-gro.cpp
===
--- clang/test/CodeGenCoroutines/coro-gro.cpp
+++ clang/test/CodeGenCoroutines/coro-gro.cpp
@@ -46,13 +46,14 @@
 // CHECK: define{{.*}} i32 @_Z1fv(
 int f() {
   // CHECK: %[[RetVal:.+]] = alloca i32
+  // CHECK: %[[GroActive:.+]] = alloca i1
 
   // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64()
   // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]])
+  // CHECK: store i1 false, ptr %[[GroActive]]
   // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeC1Ev(
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(ptr sret(%struct.GroType) align {{[0-9]+}} %[[GRO:.+]],
-  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv({{.*}}[[GRO]]
-  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(
+  // CHECK: store i1 true, ptr %[[GroActive]]
 
   Cleanup cleanup;
   doSomething();
@@ -68,7 +69,18 @@
   // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
   // CHECK: call void @_ZdlPv(ptr noundef %[[Mem]])
 
-  // CHECK: coro.ret:
+  // Initialize retval from Gro and destroy Gro
+
+  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv(
+  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: %[[IsActive:.+]] = load i1, ptr %[[GroActive]]
+  // CHECK: br i1 %[[IsActive]], label %[[CleanupGro:.+]], label %[[Done:.+]]
+
+  // CHECK: [[CleanupGro]]:
+  // CHECK:   call void @_ZN7GroTypeD1Ev(
+  // CHECK:   br label %[[Done]]
+
+  // CHECK: [[Done]]:
   // CHECK:   %[[LoadRet:.+]] = load i32, ptr %[[RetVal]]
   // CHECK:   ret i32 %[[LoadRet]]
 }
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -8103,6 +8103,12 @@
   return StmtError();
 

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 507084.
bruno added a comment.

Put dependency in place for D145641 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

Files:
  clang/include/clang/AST/StmtCXX.h
  clang/lib/AST/StmtCXX.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CodeGenCoroutines/coro-gro.cpp
  clang/test/CodeGenCoroutines/pr59221.cpp
  clang/test/SemaCXX/coroutine-no-move-ctor.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
@@ -13,8 +13,6 @@
 
 explicit task(promise_type& p) { throw 1; p.return_val = this; }
 
-task(const task&) = delete;
-
 T value;
 };
 
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -934,7 +934,7 @@
 
 extern "C" int f(mismatch_gro_type_tag2) {
   // cxx2b-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
-  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
+  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
Index: clang/test/SemaCXX/coroutine-no-move-ctor.cpp
===
--- clang/test/SemaCXX/coroutine-no-move-ctor.cpp
+++ clang/test/SemaCXX/coroutine-no-move-ctor.cpp
@@ -15,10 +15,13 @@
   };
   using promise_type = invoker_promise;
   invoker() {}
-  invoker(const invoker &) = delete;
-  invoker =(const invoker &) = delete;
-  invoker(invoker &&) = delete;
-  invoker =(invoker &&) = delete;
+  // TODO: implement RVO for get_return_object type matching
+  // function return type.
+  //
+  // invoker(const invoker &) = delete;
+  // invoker =(const invoker &) = delete;
+  // invoker(invoker &&) = delete;
+  // invoker =(invoker &&) = delete;
 };
 
 invoker f() {
Index: clang/test/CodeGenCoroutines/pr59221.cpp
===
--- clang/test/CodeGenCoroutines/pr59221.cpp
+++ clang/test/CodeGenCoroutines/pr59221.cpp
@@ -2,7 +2,7 @@
 //
 // REQUIRES: x86-registered-target
 //
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O1 -S -emit-llvm -o - | FileCheck %s
 
 #include "Inputs/coroutine.h"
 
Index: clang/test/CodeGenCoroutines/coro-gro.cpp
===
--- clang/test/CodeGenCoroutines/coro-gro.cpp
+++ clang/test/CodeGenCoroutines/coro-gro.cpp
@@ -46,13 +46,14 @@
 // CHECK: define{{.*}} i32 @_Z1fv(
 int f() {
   // CHECK: %[[RetVal:.+]] = alloca i32
+  // CHECK: %[[GroActive:.+]] = alloca i1
 
   // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64()
   // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]])
+  // CHECK: store i1 false, ptr %[[GroActive]]
   // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeC1Ev(
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(ptr sret(%struct.GroType) align {{[0-9]+}} %[[GRO:.+]],
-  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv({{.*}}[[GRO]]
-  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(
+  // CHECK: store i1 true, ptr %[[GroActive]]
 
   Cleanup cleanup;
   doSomething();
@@ -68,7 +69,18 @@
   // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
   // CHECK: call void @_ZdlPv(ptr noundef %[[Mem]])
 
-  // CHECK: coro.ret:
+  // Initialize retval from Gro and destroy Gro
+
+  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv(
+  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: %[[IsActive:.+]] = load i1, ptr %[[GroActive]]
+  // CHECK: br i1 %[[IsActive]], label %[[CleanupGro:.+]], label %[[Done:.+]]
+
+  // CHECK: [[CleanupGro]]:
+  // CHECK:   call void @_ZN7GroTypeD1Ev(
+  // CHECK:   br label %[[Done]]
+
+  // CHECK: [[Done]]:
   // CHECK:   %[[LoadRet:.+]] = load i32, ptr %[[RetVal]]
   // CHECK:   ret i32 %[[LoadRet]]
 }
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -8103,6 +8103,12 @@
   return StmtError();
 Builder.Deallocate = DeallocRes.get();
 
+

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> @bruno, @ChuanqiXu please let us know if you have any objections, otherwise 
> we will land the revert in ~2 hours.

No sweat, I didn't see this in time. Thanks for the reduced testcase.

> What's the resolution here? Can we revert this and continue the discussions 
> independently?

Eager initialization breaks us, lack of RVO on matching types breaks others. 
Landing this + D145641  altogether seems like 
the best approach.

> We can always re-land this change if the conclusion is that the approach here 
> is the one that we want.

Once we wrap up D145641  I'll land both, as 
separated commits but in the same push.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I suspect D145641  won't land today in the EU 
working hours.
My suggestion would be to revert this patch and reland together with D145641 
.
This should be in line with @ChuanqiXu's suggestions to wait for a response 
today.

@bruno, @ChuanqiXu please let us know if you have any objections, otherwise we 
will land the revert in ~2 hours.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bogdan Graur via Phabricator via cfe-commits]

bgraur added a comment.

What's the resolution here? Can we revert this and continue the discussions 
independently?
We can always re-land this change if the conclusion is that the approach here 
is the one that we want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D145639#4198837 , @ilya-biryukov 
wrote:

> In D145639#4198794 , @ChuanqiXu 
> wrote:
>
>>> ! In D145639#4198815 , @ChuanqiXu 
>>> wrote:
>>>  The RVO seems to be mandated by the standard.
>>
>> Yeah, I thought so... But WG21 decided to not make it in a recent meeting... 
>> So we can only make it as a non-mandated compiler optimization.
>
> That's unexpected. Are there any wording change proposals published?
> Our understanding comes from the reading of dcl.fct.def.coroutine/7 
> 
>
>> The expression promise.get_return_object() is used to initialize the glvalue 
>> result or prvalue result object of a call to a coroutine.
>
> And we **think** RVO is mandated when initializing the **result objects** 
> mentioned in general.
> Also, from https://github.com/llvm/llvm-project/issues/56532:
>
>> Q: if the initialization does occur later, by what mechanism the prvalue 
>> result of get_return_object is forwarded to that initialization.
>> A: (see below)
>> Case 1/2. Same type of get_return_object and coroutine return type.
>> **Constructed in the return slot.**
>> Case 3. Different types:
>> (1) Constructed temporary object prior to initial suspend initialized with a 
>> call to get_return_object()
>> (2) when coroutine needs to to return to the caller and needs to construct 
>> return value for the coroutine it is initialized with expiring value of the 
>> temporary obtained in step 1.

The formal file shouldn't be released yet. But we can find the issue report and 
the proposed solution in https://cplusplus.github.io/CWG/issues/2563.html.

The words you quoted is the recording of the wg21's discussion during the 
meeting. And that's the **intention** of the designers but it is not reflected 
to the wording.  What issue 2563 discussed is that if we should delay the 
conversion. And the RVO is a conclusion of that. But according to the result, 
we should say the compiler is allowed to do RVO if the types matched but it is 
not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D145639#4198794 , @ChuanqiXu wrote:

> Reverting is common in Clang/LLVM. But let's wait for the response from 
> @bruno temporarily. I think it makes sense to revert this one if @bruno don't 
> response tomorrow. And we can commit this one and D145641 
>  quickly the next time. Recommit is common 
> too.

@bgraur, FYI, hope waiting for a day works for this case.

In D145639#4198815 , @ChuanqiXu wrote:

>> The RVO seems to be mandated by the standard.
>
> Yeah, I thought so... But WG21 decided to not make it in a recent meeting... 
> So we can only make it as a non-mandated compiler optimization.

That's unexpected. Are there any wording change proposals published?
Our understanding comes from the reading of dcl.fct.def.coroutine/7 


> The expression promise.get_return_object() is used to initialize the glvalue 
> result or prvalue result object of a call to a coroutine.

And we **think** RVO is mandated when initializing the **result objects** 
mentioned in general.
Also, from https://github.com/llvm/llvm-project/issues/56532:

> Q: if the initialization does occur later, by what mechanism the prvalue 
> result of get_return_object is forwarded to that initialization.
> A: (see below)
> Case 1/2. Same type of get_return_object and coroutine return type.
> **Constructed in the return slot.**
> Case 3. Different types:
> (1) Constructed temporary object prior to initial suspend initialized with a 
> call to get_return_object()
> (2) when coroutine needs to to return to the caller and needs to construct 
> return value for the coroutine it is initialized with expiring value of the 
> temporary obtained in step 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> The RVO seems to be mandated by the standard.

Yeah, I thought so... But WG21 decided to not make it in a recent meeting... So 
we can only make it as a non-mandated compiler optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

The reproducer seems like what we're searching for tests in 
https://reviews.llvm.org/D145641. Reverting is common in Clang/LLVM. But let's 
wait for the response from @bruno temporarily. I think it makes sense to revert 
this one if @bruno don't response tomorrow. And we can commit this one and 
D145641  quickly the next time. Recommit is 
common too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Reverting the RVO breaks some coroutine code we have. The short repro is  
https://gcc.godbolt.org/z/1543qc8Ee (code at the end of the post, very similar 
to the deleted constructor in `warn-throw-out-noexcept-coro.cpp`):
The RVO seems to be mandated by the standard and D145641 
 is in the works to fix this.

Could we revert this change and wait for D145641 
 to land before reverting existing RVO 
behavior?
This would unblock our compiler releases (we live at head). This looks like the 
right trade-off given that previous behavior was in Clang for more than a year 
after D117087  landed.

Reproducer  (works in Clang 15 and GCC, 
fails in trunk):

  #include 
  
  struct MyTask{
struct promise_type {
  MyTask get_return_object() { return 
MyTask{std::coroutine_handle::from_promise(*this)}; }
  std::suspend_always initial_suspend() { return {}; }
  
  void unhandled_exception();
  void return_void() {} 
  
  auto await_transform(MyTask task) {
struct Awaiter {
  bool await_ready() { return false; }
  std::coroutine_handle 
await_suspend(std::coroutine_handle h);
  std::nullptr_t await_resume();
  
  promise_type& caller;
  promise_type& callee;
};
  
return Awaiter{*this, task.handle.promise()};
  }
  
  auto final_suspend() noexcept {
struct Awaiter {
  bool await_ready() noexcept { return false; }
  std::coroutine_handle 
await_suspend(std::coroutine_handle h) noexcept;
  void await_resume() noexcept;
  
  std::coroutine_handle to_resume;
};
  
return Awaiter{resume_when_done};
  }
  
  // The coroutine to resume when we're done.
  std::coroutine_handle resume_when_done;
};
  
explicit MyTask(std::coroutine_handle);
MyTask(MyTask&&) = delete;
  
// A handle for the coroutine that returned this task.
std::coroutine_handle handle;
  };
  
  MyTask DoNothing() {
co_return;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG54225c457a33: [Coroutines] Fix premature conversion of 
return object (authored by bruno).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

Files:
  clang/include/clang/AST/StmtCXX.h
  clang/lib/AST/StmtCXX.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CodeGenCoroutines/coro-gro.cpp
  clang/test/CodeGenCoroutines/pr59221.cpp
  clang/test/SemaCXX/coroutine-no-move-ctor.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
@@ -13,8 +13,6 @@
 
 explicit task(promise_type& p) { throw 1; p.return_val = this; }
 
-task(const task&) = delete;
-
 T value;
 };
 
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -934,7 +934,7 @@
 
 extern "C" int f(mismatch_gro_type_tag2) {
   // cxx2b-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
-  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
+  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
Index: clang/test/SemaCXX/coroutine-no-move-ctor.cpp
===
--- clang/test/SemaCXX/coroutine-no-move-ctor.cpp
+++ clang/test/SemaCXX/coroutine-no-move-ctor.cpp
@@ -15,10 +15,13 @@
   };
   using promise_type = invoker_promise;
   invoker() {}
-  invoker(const invoker &) = delete;
-  invoker =(const invoker &) = delete;
-  invoker(invoker &&) = delete;
-  invoker =(invoker &&) = delete;
+  // TODO: implement RVO for get_return_object type matching
+  // function return type.
+  //
+  // invoker(const invoker &) = delete;
+  // invoker =(const invoker &) = delete;
+  // invoker(invoker &&) = delete;
+  // invoker =(invoker &&) = delete;
 };
 
 invoker f() {
Index: clang/test/CodeGenCoroutines/pr59221.cpp
===
--- clang/test/CodeGenCoroutines/pr59221.cpp
+++ clang/test/CodeGenCoroutines/pr59221.cpp
@@ -2,7 +2,7 @@
 //
 // REQUIRES: x86-registered-target
 //
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O1 -S -emit-llvm -o - | FileCheck %s
 
 #include "Inputs/coroutine.h"
 
Index: clang/test/CodeGenCoroutines/coro-gro.cpp
===
--- clang/test/CodeGenCoroutines/coro-gro.cpp
+++ clang/test/CodeGenCoroutines/coro-gro.cpp
@@ -46,13 +46,14 @@
 // CHECK: define{{.*}} i32 @_Z1fv(
 int f() {
   // CHECK: %[[RetVal:.+]] = alloca i32
+  // CHECK: %[[GroActive:.+]] = alloca i1
 
   // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64()
   // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]])
+  // CHECK: store i1 false, ptr %[[GroActive]]
   // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeC1Ev(
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(ptr sret(%struct.GroType) align {{[0-9]+}} %[[GRO:.+]],
-  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv({{.*}}[[GRO]]
-  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(
+  // CHECK: store i1 true, ptr %[[GroActive]]
 
   Cleanup cleanup;
   doSomething();
@@ -68,7 +69,18 @@
   // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
   // CHECK: call void @_ZdlPv(ptr noundef %[[Mem]])
 
-  // CHECK: coro.ret:
+  // Initialize retval from Gro and destroy Gro
+
+  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv(
+  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: %[[IsActive:.+]] = load i1, ptr %[[GroActive]]
+  // CHECK: br i1 %[[IsActive]], label %[[CleanupGro:.+]], label %[[Done:.+]]
+
+  // CHECK: [[CleanupGro]]:
+  // CHECK:   call void @_ZN7GroTypeD1Ev(
+  // CHECK:   br label %[[Done]]
+
+  // CHECK: [[Done]]:
   // CHECK:   %[[LoadRet:.+]] = load i32, ptr %[[RetVal]]
   // CHECK:   ret i32 %[[LoadRet]]
 }
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ 

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Test failures are unrelated, thanks for the review @ChuanqiXu


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

This is basically a reverting of https://reviews.llvm.org/D117087. So it should 
be good according to our previous talk.




Comment at: clang/include/clang/AST/StmtCXX.h:410-414
   Expr *getReturnValue() const {
 assert(getReturnStmt());
 auto *RS = cast(getReturnStmt());
 return RS->getRetValue();
   }

Maybe we can delete this.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1752
 
-  StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+  // StmtResult ReturnStmt = S.BuildReturnStmt(Loc, ReturnValue);
+  auto *GroDecl = VarDecl::Create(

Let's remove this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145639

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

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added a reviewer: ChuanqiXu.
Herald added subscribers: hoy, modimo, wenlei.
Herald added a project: All.
bruno requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix https://github.com/llvm/llvm-project/issues/56532

Effectively, this reverts behavior introduced in 
https://reviews.llvm.org/D117087,
which did two things:

1. Change delayed to early conversion of return object.
2. Introduced RVO possibilities because of early conversion.

This patches fixes (1) and removes (2). I already worked on a follow up for (2)
in a separated patch. I believe it's important to split these two because if 
the RVO
causes any problems we can explore reverting (2) while maintaining (1).

Notes on some testcase changes:

- `pr59221.cpp` changed to `-O1` so we can check that the front-end honors the 
value checked for. Sounds like `-O3` without RVO is more likely to work with 
LLVM optimizations...
- Comment out delete members `coroutine-no-move-ctor.cpp` since behavior now 
requires copies again.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145639

Files:
  clang/include/clang/AST/StmtCXX.h
  clang/lib/AST/StmtCXX.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CodeGenCoroutines/coro-gro.cpp
  clang/test/CodeGenCoroutines/pr59221.cpp
  clang/test/SemaCXX/coroutine-no-move-ctor.cpp
  clang/test/SemaCXX/coroutines.cpp
  clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp

Index: clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
===
--- clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
+++ clang/test/SemaCXX/warn-throw-out-noexcept-coro.cpp
@@ -13,8 +13,6 @@
 
 explicit task(promise_type& p) { throw 1; p.return_val = this; }
 
-task(const task&) = delete;
-
 T value;
 };
 
Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -934,7 +934,7 @@
 
 extern "C" int f(mismatch_gro_type_tag2) {
   // cxx2b-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
-  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an rvalue of type 'void *'}}
+  // cxx14_20-error@-2 {{cannot initialize return object of type 'int' with an lvalue of type 'void *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
Index: clang/test/SemaCXX/coroutine-no-move-ctor.cpp
===
--- clang/test/SemaCXX/coroutine-no-move-ctor.cpp
+++ clang/test/SemaCXX/coroutine-no-move-ctor.cpp
@@ -15,10 +15,13 @@
   };
   using promise_type = invoker_promise;
   invoker() {}
-  invoker(const invoker &) = delete;
-  invoker =(const invoker &) = delete;
-  invoker(invoker &&) = delete;
-  invoker =(invoker &&) = delete;
+  // TODO: implement RVO for get_return_object type matching
+  // function return type.
+  //
+  // invoker(const invoker &) = delete;
+  // invoker =(const invoker &) = delete;
+  // invoker(invoker &&) = delete;
+  // invoker =(invoker &&) = delete;
 };
 
 invoker f() {
Index: clang/test/CodeGenCoroutines/pr59221.cpp
===
--- clang/test/CodeGenCoroutines/pr59221.cpp
+++ clang/test/CodeGenCoroutines/pr59221.cpp
@@ -2,7 +2,7 @@
 //
 // REQUIRES: x86-registered-target
 //
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O3 -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -O1 -S -emit-llvm -o - | FileCheck %s
 
 #include "Inputs/coroutine.h"
 
Index: clang/test/CodeGenCoroutines/coro-gro.cpp
===
--- clang/test/CodeGenCoroutines/coro-gro.cpp
+++ clang/test/CodeGenCoroutines/coro-gro.cpp
@@ -46,13 +46,14 @@
 // CHECK: define{{.*}} i32 @_Z1fv(
 int f() {
   // CHECK: %[[RetVal:.+]] = alloca i32
+  // CHECK: %[[GroActive:.+]] = alloca i1
 
   // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64()
   // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]])
+  // CHECK: store i1 false, ptr %[[GroActive]]
   // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_typeC1Ev(
-  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(ptr sret(%struct.GroType) align {{[0-9]+}} %[[GRO:.+]],
-  // CHECK: %[[Conv:.+]] = call noundef i32 @_ZN7GroTypecviEv({{.*}}[[GRO]]
-  // CHECK: store i32 %[[Conv]], ptr %[[RetVal]]
+  // CHECK: call void @_ZNSt16coroutine_traitsIJiEE12promise_type17get_return_objectEv(
+  // CHECK: store i1 true, ptr %[[GroActive]]
 
   Cleanup cleanup;
   doSomething();
@@ -68,7 +69,18 @@
   // CHECK: %[[Mem:.+]] = call ptr @llvm.coro.free(
   // CHECK: call