[PATCH] D22603: [coroutines] Part 1 of N: Documentation

[David Majnemer via llvm-commits]

majnemer added a comment.

In https://reviews.llvm.org/D22603#491320, @GorNishanov wrote:

> In https://reviews.llvm.org/D22603#490812, @majnemer wrote:
>
> > In https://reviews.llvm.org/D22603#490753, @GorNishanov wrote:
> >
> > > In https://reviews.llvm.org/D22603#490604, @majnemer wrote:
> > >
> > > > Out of curiosity, what lead you to use tokens for `coro.save` and `coro.suspend`?
> > >
> > >
> > > [snip] Besides, the only purpose of return value of coro.save is to give it to matching coro.suspend. It does not have any other meaning, besides providing a facility of linking those two intrinsics together.
> >
> >
> > OK, so it is critical that you be able to find the coro.suspend with its associated coro.save?
>
>
> Yes, for example, given a coro.suspend describing a suspend/resume point 5, I need to find its coro save and replace it with:
>
>   %index.addr = gep %hdl <index-field>
>   store 5, index.addr 
>   
>
> The motivating example why a suspend point is split into two instructions, save and suspend is mentioned in the description of coro.save intrinsic.
>
>   %save1 = call token @llvm.coro.save(i8* %hdl)
>   call void async_op1(i8* %hdl)
>   %suspend1 = call i1 @llvm.coro.suspend(token %save1, i1 false)
>   switch i8 %suspend1, label %suspend [i8 0, label %resume1
>                                        i8 1, label %cleanup]
>   
>
> It will get lowered to:
>
>   %index.addr = gep %hdl <index-field>
>   store 1, index.addr 
>   call void async_op1(i8* %hdl)
>   ret void
>   
>
> Preparing coroutine for suspension, (just saving the current suspend point index in the coroutine frame), allows the coroutine to be resumed by async_op from the current or a different thread.
>
> If we did not have coro.save (and were saving the current suspend point index at the point of coro.suspend), then, the store to the index will race with possible resumption and chaos and mayhem will ensue. :-)


OK, sounds like a great use of token!


================
Comment at: docs/Coroutines.rst:659
@@ +658,3 @@
+      declare <type>* @llvm.coro.promise.p0<type>(i8* <handle>)
+
+Overview:
----------------
GorNishanov wrote:
> majnemer wrote:
> > I was suggesting:
> >   declare i8* @llvm.coro.promise(i8* <handle>)
> The coro.promise / from.promise intrinsics hide the structure of the coroutine frame. To figure out the offset from where the coroutine handle points to where promise lives, LLVM needs to know its type (or more precisely alignment).
> 
> Let's say we are on the architecture with 4 byte pointers, but, the promise requires 16 byte alignment. In this case, the coroutine frame might be:
> 
> ```
> 0000          0004           008                 0016
> [ResumeFnPtr] [DestroyFnPtr] less aligned things [promise]
>                              or padding
> ```
> 
> Given that we don't really care about the type and only need to know promise alignment, I take your suggestion and counter offer the following as a replacement for both coro.promise and coro.from.promise intrinsics.
> 
> ```
> decl i8* @llvm.coro.promise(i8* <handle>, i32 <promise-alignment>, i1 <from>)
> 
> old model                            new model
> ---------                            ---------
> @llvm.coro.promise.p0i32(%hdl)       @llvm.coro.promise(%hdl, 4, false)
> @llvm.coro.from.promise.p0i32(%hdl)  @llvm.coro.promise(%hdl, 4, true)
> ```
> 
> Better now?
Looks great.


https://reviews.llvm.org/D22603