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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
