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

[Gor Nishanov via llvm-commits]

GorNishanov added a comment.

In https://reviews.llvm.org/D22659#495774, @majnemer wrote:

> In https://reviews.llvm.org/D22659#495738, @GorNishanov wrote:
>
> > coro.begin is now capturing the promise parameter, otherwise, GVN won't recognize operations that may write to the promise.
>
>
> coro.begin semantically stores the promise parameter somewhere? The semantics portion of the coro.begin intrinsic doesn't describe what happens when the promise parameter is provided.


coro.begin does not do anything with the promise on lowering, apart from letting the promise alloca "escape" due to taking it as an argument. 
This property comes from the semantic of the promise itself. Promise alloca is directly accessible from outside of the coroutine.

The issue I ran into was that in expected.cpp: https://github.com/GorNishanov/clang/blob/coro/test/Coroutines/expected.cpp, await_suspend stores the error value in the promise.

  void await_suspend(std::coroutine_handle<promise_type> h) { 
    h.promise().data.error =std::move(data.error); 
    h.destroy(); 
  } 

To llvm it looks like:

  %4 = call noalias nonnull i8* @llvm.coro.begin(i8* nonnull %3, i32 0, i8* %promise, i8* null)
  ...
  ; inlined from call await_suspend(%awaiter_this, %4)
  %error4.i = getelementptr inbounds i8, i8* %4, i32 16
  %5 = bitcast i8* %error4.i to i32*
  store i32 42, i32* %5, align 4, !tbaa !15 ; stores into promise, 
                                            ; GVN did not know that promise escaped due to NoCapture in coro.begin
                                            ; and thought in the block below that the error value is undef
  ...
  AfterCoroEnd:                                     ; Copy expected<int,int> back to result of the function
    %val.i = getelementptr inbounds %struct.expected, %struct.expected* %agg.result, i32 0, i32 0, i32 0
    %val4.i = getelementptr inbounds %"struct.expected<int, int>::promise_type", %"struct.expected<int, int>::promise_type"* %promise, i32 0, i32 0, i32 0
    %9 = load i32, i32* %val4.i, align 4, !tbaa !14
    store i32 %9, i32* %val.i, align 4, !tbaa !1
    %error.i = getelementptr inbounds %struct.expected, %struct.expected* %agg.result, i32 0, i32 0, i32 1
    %error6.i = getelementptr inbounds %"struct.expected<int, int>::promise_type", %"struct.expected<int, int>::promise_type"* %promise, i32 0, i32 0, i32 1
    %10 = load i32, i32* %error6.i, align 4, !tbaa !14
    store i32 %10, i32* %error.i, align 4, !tbaa !6
    ret void
  }




================
Comment at: include/llvm/IR/Intrinsics.td:605
@@ +604,3 @@
+
+def int_coro_alloc : Intrinsic<[llvm_ptr_ty], [], []>;
+def int_coro_begin : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty, llvm_i32_ty,
----------------
majnemer wrote:
> Each coro.alloc is neither a distinct alloca or null right? I think we can mark the function return with `noalias`.
I am not sure I can do this with within an Intrinsics.td . NoAlias is not defined there.
Currently, I am adding NoAlias and NotNull attributes to the coro.begin calls in CoroEarly pass.


https://reviews.llvm.org/D22659