[PATCH] D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws

[John McCall via Phabricator via cfe-commits]

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

Minor suggestion in the docs, but otherwise LGTM.



================
Comment at: llvm/docs/Coroutines.rst:1382
+The `coro.end` intrinsic in the normal path wouldn't do this since the coroutine
+should already be marked as done by final suspend.
+
----------------
How about:

  In the unwind path (when the argument is `true`), `coro.end` will mark the coroutine as done, making it undefined behavior to resume the coroutine again and causing `llvm.coro.done` to return `true`.  This is not necessary in the normal path because the coroutine will already be marked as done by the final suspend.


================
Comment at: llvm/docs/Coroutines.rst:1395
+|            | Landingpad  | mark coroutine as done | mark coroutine done           |
++------------+-------------+------------------------+-------------------------------+
 
----------------
ChuanqiXu wrote:
> Maybe I need to explain why it would work in start (ramp) function. Generally, the ramp function is very small. But in case the initial_suspend of the coroutine wouldn't always suspend, the start function could be as large as the original function. In this case, it should remain the original semantics.
That makes sense.  Honestly, it feels very strange to me that there are any differences between the ramp function and the resume/destroy functions in the handling of `coro.end`.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:304
+        Shape.FrameTy->getTypeAtIndex(coro::Shape::SwitchFieldIndex::Resume)));
+    Builder.CreateStore(NullPtr, GepIndex);
     if (!InResume)
----------------
ChuanqiXu wrote:
> rjmccall wrote:
> > We need to do this store elsewhere, right, like during final suspends?  Can we make a common function for it?
> I am not sure if I understand your point. I think the place where the `coro.end` lives should be the right place. The common function is made.
Yes, thank you, this is all I wanted.


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

https://reviews.llvm.org/D115219