[PATCH] D22659: [coroutines] Part 2 of N: Adding Coroutine Intrinsics

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: include/llvm/IR/Intrinsics.td:606-607
@@ +605,4 @@
+def int_coro_alloc : Intrinsic<[llvm_ptr_ty], [], [IntrNoMem]>;
+def int_coro_begin : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty, llvm_i32_ty,
+                                llvm_ptr_ty, llvm_ptr_ty], []>;
+
----------------
GorNishanov wrote:
> majnemer wrote:
> > What happens if there are two calls to llvm.coro.begin in a function?
> **TLDR: **
>   coro.begin(*,*,*, null) - must have exactly one, broken IR otherwise
>   coro.begin(*,*,*, not-null) - can have as many or as little as inliner wants
> 
> **Longer explanation:**
> 
> The last argument of coro.begin is null when coming out of the frontend. Later, CoroSplit pass sets it to a pointer to a constant array containing pointers to outlined parts of the coroutine. Let's call a coro.begin with null "pre-slit coro.begin", and the one with a pointer to const array, "post-split coro.begin".
> 
> Coming out of the frontend, a coroutine has exactly one coro.begin. (Verifier can check for that). Later inliner can inline calls to other coroutines and it may bring more coro.begins into a function, but, those, will be always post-split, since CoroSplit pass runs together with the Inliner in the same CGPassManager.
> 
> CoroSplit pass only cares about pre-split coro.begin and there is exactly one. Other coro.begins are there for the benefit of CoroElide pass that devirtualizes and elides heap allocations where possible.
> 
> All intrinsics that stay in the coroutine after CoroSplit pass (coro.alloc and coro.free) are linked to coro.begin, so CoroElide pass knows which coroutine they belong to.
Can't earlier transforms clone a block containing a `coro.begin` call?

================
Comment at: include/llvm/IR/Intrinsics.td:609
@@ +608,3 @@
+
+def int_coro_free : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty], [IntrNoMem]>;
+def int_coro_end : Intrinsic<[], [llvm_ptr_ty, llvm_i1_ty], []>;
----------------
GorNishanov wrote:
> majnemer wrote:
> > I don't know if `IntrNoMem` is right here but `NoCapture<0>` is probably fine.
> Ack on NoCapture<0>. 
> With respect to IntrNoMem my thinking was:
> 
> coro.free(%hdl) can be lowered in one of three ways:
> 
> 1. null - if CoroElide pass elides dynamic allocations
> 2. %hdl - if coroutine handle points to the beginning of the memory block
> 3. gep on %hdl - that adjusts the pointer to point to the beginning of the memory block.
> 
> Wait, you are right, gep may not be a constant gep, if some of the allocas in the coroutine have alignment requirements that exceed the alignment guarantees provided by the allocation function (second parameter to coro.begin), then, indeed, coro.free would have to read some adjustment value from the memory pointed by handle to do the correct adjustment.
> 
> Okay, then, 
> ```
> [IntrReadMem, NoCapture<0>]
> ```
> it is.
SGTM

================
Comment at: include/llvm/IR/Intrinsics.td:618-620
@@ +617,5 @@
+
+def int_coro_param : Intrinsic<[llvm_i1_ty], [llvm_ptr_ty, llvm_ptr_ty],
+                               [IntrReadMem, ReadNone<0>, NoCapture<0>,
+                                ReadNone<1>,NoCapture<1>]>;
+
----------------
GorNishanov wrote:
> majnemer wrote:
> > How is coro.param sensitive to non-parameter stores?
> To double check:
> 
> You are asking why [IntrReadMem] and not [IntrNoMem]?
> 
> I think it is safe to mark it [IntrNoMem].
> It is always lowered to a Boolean constant, and the only purpose for the first and second argument is to tie those two allocas together, in case, I want to elide parameter copy and need to RAUW one with another.
Cool, IntrNoMem sounds good to me.

================
Comment at: include/llvm/IR/Intrinsics.td:633-634
@@ +632,4 @@
+
+def int_coro_subfn_addr : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty, llvm_i8_ty],
+                                    [IntrReadMem]>;
+
----------------
GorNishanov wrote:
> majnemer wrote:
> > Is it possible for optimizations to come across this intrinsic.  If so, what semantics does it have?
> The life of coro.subfn.addr looks like this:
> 
> CoroEarly replaces:
> 
> ```
>    call i8* llvm.coro.resume(i8* %Arg)
> ```
> with
> ```
>    %0 = call i8* llvm.coro.subfn.addr(i8* %Arg, i8 0)
>    %1 = bitcast i8* %0 to void(i8*)*
>    call fastcc void %1 (i8* %Arg)
> ```
> 
> CoroElide may replace coro.subfn.addr with a f.resume function pointer, so it will be constant folded to:
> 
> ```
>    call fastcc void @f.resume (i8* %Arg)
> ```
> 
> If CoroElide wasn't able to do devirtualize it, it will stay intact until CoroCleanup pass run at EP_OptimizerLast. CoroCleanup will replace it with: gep + load to get the address of the appropriate resume function at runtime.
> 
> 0 - f.resume
> 1 - f.destroy
> 
> P.S.
> 
> Frontend does not emit coro.subfn.addr directly, as we would like to hide the details of the calling convention for the coroutine sub-functions.
> 
Ah, ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D22659