[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

In D87470#2273310 , @junparser wrote:

> @lxfind , could you backport this to branch 11?

I am actually seeing some problems with this change. Still investigating.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[JunMa via Phabricator via cfe-commits]

junparser added a comment.
Herald added a subscriber: modimo.

@lxfind , could you backport this to branch 11?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li 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 rGdf477db5f9e0: [Coroutine][Sema] Tighten the lifetime of 
symmetric transfer returned handle (authored by lxfind).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/Inputs/coroutine.h
  clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -emit-llvm -S -O1 %s -o -
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return 
detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> 
await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// check that the lifetime of the coroutine handle used to obtain the address 
ended right away.
+// CHECK:   %{{.*}} = call i8* 
@{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"*
 nonnull %{{.*}})
+// CHECK-NEXT:  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %{{.*}})
Index: clang/test/CodeGenCoroutines/Inputs/coroutine.h
===
--- clang/test/CodeGenCoroutines/Inputs/coroutine.h
+++ clang/test/CodeGenCoroutines/Inputs/coroutine.h
@@ -15,7 +15,7 @@
 return me;
   }
   void operator()() { resume(); }
-  void *address() const { return ptr; }
+  void *address() const noexcept { return ptr; }
   void resume() const { __builtin_coro_resume(ptr); }
   void destroy() const { __builtin_coro_destroy(ptr); }
   bool done() const { return __builtin_coro_done(ptr); }
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,6 +398,10 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
+  // The coroutine handle used to obtain the address is no longer needed
+  // at this point, clean it up to avoid unnecessarily long lifetime which
+  // could lead to unnecessary spilling.
+  JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
   return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
   JustAddress);
 }


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -emit-llvm -S -O1 %s -o -
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// check that the lifetime of the coroutine handle used to obtain the address ended right away.
+// CHECK:   %{{.*}} = call i8* @{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"* nonnull %{{.*}})
+// CHECK-NEXT:  call void 

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

In D87470#2268911 , @rjmccall wrote:

> Thanks, LGTM.

Thank you for reviewing and the suggestions on testcase!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[John McCall via Phabricator via cfe-commits]

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

Thanks, LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind updated this revision to Diff 291324.
lxfind added a comment.

remove asan option, not needed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/Inputs/coroutine.h
  clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -emit-llvm -S -O1 %s -o -
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return 
detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> 
await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// check that the lifetime of the coroutine handle used to obtain the address 
ended right away.
+// CHECK:   %{{.*}} = call i8* 
@{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"*
 nonnull %{{.*}})
+// CHECK-NEXT:  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %{{.*}})
Index: clang/test/CodeGenCoroutines/Inputs/coroutine.h
===
--- clang/test/CodeGenCoroutines/Inputs/coroutine.h
+++ clang/test/CodeGenCoroutines/Inputs/coroutine.h
@@ -15,7 +15,7 @@
 return me;
   }
   void operator()() { resume(); }
-  void *address() const { return ptr; }
+  void *address() const noexcept { return ptr; }
   void resume() const { __builtin_coro_resume(ptr); }
   void destroy() const { __builtin_coro_destroy(ptr); }
   bool done() const { return __builtin_coro_done(ptr); }
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,6 +398,10 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
+  // The coroutine handle used to obtain the address is no longer needed
+  // at this point, clean it up to avoid unnecessarily long lifetime which
+  // could lead to unnecessary spilling.
+  JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
   return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
   JustAddress);
 }


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -emit-llvm -S -O1 %s -o -
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// check that the lifetime of the coroutine handle used to obtain the address ended right away.
+// CHECK:   %{{.*}} = call i8* @{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"* nonnull %{{.*}})
+// CHECK-NEXT:  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %{{.*}})
Index: clang/test/CodeGenCoroutines/Inputs/coroutine.h

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind updated this revision to Diff 291321.
lxfind added a comment.

Add test to verify that lifetime.end appears right after the address call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/Inputs/coroutine.h
  clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -fsanitize=address -emit-llvm -S -O1 
%s -o -
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return 
detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> 
await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// check that the lifetime of the coroutine handle used to obtain the address 
ended right away.
+// CHECK:   %{{.*}} = call i8* 
@{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"*
 nonnull %{{.*}})
+// CHECK-NEXT:  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %{{.*}})
Index: clang/test/CodeGenCoroutines/Inputs/coroutine.h
===
--- clang/test/CodeGenCoroutines/Inputs/coroutine.h
+++ clang/test/CodeGenCoroutines/Inputs/coroutine.h
@@ -15,7 +15,7 @@
 return me;
   }
   void operator()() { resume(); }
-  void *address() const { return ptr; }
+  void *address() const noexcept { return ptr; }
   void resume() const { __builtin_coro_resume(ptr); }
   void destroy() const { __builtin_coro_destroy(ptr); }
   bool done() const { return __builtin_coro_done(ptr); }
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,6 +398,10 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
+  // The coroutine handle used to obtain the address is no longer needed
+  // at this point, clean it up to avoid unnecessarily long lifetime which
+  // could lead to unnecessary spilling.
+  JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
   return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
   JustAddress);
 }


Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -fsanitize=address -emit-llvm -S -O1 %s -o -
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// check that the lifetime of the coroutine handle used to obtain the address ended right away.
+// CHECK:   %{{.*}} = call i8* @{{.*address.*}}(%"struct.std::experimental::coroutines_v1::coroutine_handle.0"* nonnull %{{.*}})
+// CHECK-NEXT:  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %{{.*}})
Index: 

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D87470#2268267 , @lxfind wrote:

> hmm @rjmccall, I don't think there is a stable way to test this. The code 
> generated for symmetric transfer is way too complicated to stably pattern 
> match one less item in the frame.

I don't know that much about the code-generation here, but when cleanups escape 
into a surrounding scope, you can usually test that the cleanup is emitted 
before some call tied to a later statement in that scope.  You don't have to 
test for every last instruction in the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind updated this revision to Diff 291255.
lxfind added a comment.

Remove unstable test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

Files:
  clang/lib/Sema/SemaCoroutine.cpp


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,8 +398,8 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-  JustAddress);
+  return S.MaybeCreateExprWithCleanups(
+  buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume, JustAddress));
 }
 
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,8 +398,8 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-  JustAddress);
+  return S.MaybeCreateExprWithCleanups(
+  buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume, JustAddress));
 }
 
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

hmm @rjmccall, I don't think there is a stable way to test this. The code 
generated for symmetric transfer is way too complicated to stably pattern match 
one less item in the frame.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind updated this revision to Diff 291140.
lxfind added a comment.

Add test case. Verify that the size of the frame reduced by 8.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/CodeGenCoroutines/Inputs/coroutine.h
  clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp

Index: clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-semmetric-transfer.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang -std=c++14 -fcoroutines-ts -fsanitize=address -emit-llvm -S -o - %s | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct detached_task {
+  struct promise_type {
+detached_task get_return_object() noexcept {
+  return detached_task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return coro::noop_coroutine();
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+  };
+
+  ~detached_task() {
+if (coro_) {
+  coro_.destroy();
+  coro_ = {};
+}
+  }
+
+  void start() && {
+auto tmp = coro_;
+coro_ = {};
+tmp.resume();
+  }
+
+  coro::coroutine_handle coro_;
+};
+
+detached_task foo() {
+  co_return;
+}
+
+// This is to check that temporary handle returned by the symmetric transfer is not put in the frame.
+// CHECK: {{.*}} = call nonnull i8* @_Znwm(i64 48)
Index: clang/test/CodeGenCoroutines/Inputs/coroutine.h
===
--- clang/test/CodeGenCoroutines/Inputs/coroutine.h
+++ clang/test/CodeGenCoroutines/Inputs/coroutine.h
@@ -15,7 +15,7 @@
 return me;
   }
   void operator()() { resume(); }
-  void *address() const { return ptr; }
+  void *address() const noexcept { return ptr; }
   void resume() const { __builtin_coro_resume(ptr); }
   void destroy() const { __builtin_coro_destroy(ptr); }
   bool done() const { return __builtin_coro_done(ptr); }
@@ -52,19 +52,21 @@
   }
 };
 
-  template 
-  bool operator==(coroutine_handle<_PromiseT> const& _Left,
-coroutine_handle<_PromiseT> const& _Right) noexcept
-  {
-return _Left.address() == _Right.address();
-  }
+struct noop_coroutine_promise {};
+using noop_coroutine_handle = coroutine_handle;
+coroutine_handle<> noop_coroutine() { return {}; }
 
-  template 
-  bool operator!=(coroutine_handle<_PromiseT> const& _Left,
-coroutine_handle<_PromiseT> const& _Right) noexcept
-  {
-return !(_Left == _Right);
-  }
+template 
+bool operator==(coroutine_handle<_PromiseT> const &_Left,
+coroutine_handle<_PromiseT> const &_Right) noexcept {
+  return _Left.address() == _Right.address();
+}
+
+template 
+bool operator!=(coroutine_handle<_PromiseT> const &_Left,
+coroutine_handle<_PromiseT> const &_Right) noexcept {
+  return !(_Left == _Right);
+}
 
 struct suspend_always {
   bool await_ready() { return false; }
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,8 +398,8 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-  JustAddress);
+  return S.MaybeCreateExprWithCleanups(
+  buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume, JustAddress));
 }
 
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Also LGTM with a testcase.  It's fine to test the result of IRGen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

In D87470#2267060 , @junparser wrote:

> Thanks for the change. LGTM, and testcase？

Not sure how to add a test case for this though. We don't seem to explicitly 
test AST output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[JunMa via Phabricator via cfe-commits]

junparser added a comment.

Thanks for the change. LGTM, and testcase？


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87470

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

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

[Xun Li via Phabricator via cfe-commits]

lxfind created this revision.
lxfind added reviewers: lewissbaker, wenlei, hoy, bruno, junparser, rjmccall.
Herald added subscribers: cfe-commits, dexonsmith, modocache.
Herald added a project: clang.
lxfind requested review of this revision.

In generating the code for symmetric transfer, a temporary object is created to 
store the returned handle from await_suspend() call of the awaiter. Previously 
this temp won't be cleaned up until very later, which ends up causing this temp 
to be spilled to the heap. However, we know that this temp will no longer be 
needed after the coro_resume call. We can clean it up right after.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87470

Files:
  clang/lib/Sema/SemaCoroutine.cpp


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,8 +398,8 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-  JustAddress);
+  return S.MaybeCreateExprWithCleanups(
+  buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume, JustAddress));
 }
 
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await


Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -398,8 +398,8 @@
diag::warn_coroutine_handle_address_invalid_return_type)
 << JustAddress->getType();
 
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-  JustAddress);
+  return S.MaybeCreateExprWithCleanups(
+  buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume, JustAddress));
 }
 
 /// Build calls to await_ready, await_suspend, and await_resume for a co_await
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits