[PATCH] D100415: [Coroutines] Split coroutine during CoroEarly into an init and ramp function

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

It looks like this code may trigger assertion in `CoroInstr.h` for `CoroIdInst::setCoroutineSelf`.

Also, since this patch would enlarge the coroutine frame, it may affect the performance naturally. I **believe**  it wouldn't really matter. I just find that we need coroutine benchmarks which seems missing now. It is really needed when we talk about the performance for coroutine. Although our intuition tell us it should be ok, we still need some data to approve us instead of imaging. I am not asking for benchmark or score for this patch. I just find there is no benchmark we can evaluate the performance for coroutine. I believe it would be a future direction.

Then, since there is hidden chance to decrease the performance, I wonder is it better to give an option to control how coroutine handle the parameters. We can use strategy in this patch by default. An option could help us in tuning the performance in the future.

I would try to look into the details for the code.



================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:619
+    EmitBlock(InitBB);
+    SmallVector<llvm::AllocaInst *, 4> FrameAllocas;
     // Create parameter copies. We do it before creating a promise, since an
----------------
Did this vector have been used?


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:638
+                  llvm::ConstantAsMetadata::get(Builder.getInt32(ID++)),
+              }));
       // TODO: if(CoroParam(...)) need to surround ctor and dtor
----------------
I wonder if it is better to document the metadata `coroutine_frame_alloca` in somewhere like the metadata `tbaa`.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:646
 
+    Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::coro_init_end));
+    Builder.CreateBr(InitReadyBB);
----------------
It calls `coro.init.end` without calling `coro.init` in the front which looks odd.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:153
 
+static void splitRampFunction(Function &F) {
+  Module *M = F.getParent();
----------------
We need comment for the intention of the function.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:156
+  LLVMContext &C = M->getContext();
+  {
+    CoroBeginInst *CoroBegin = cast<CoroBeginInst>(
----------------
It looks odd for several `{}` in Function to avoid name collision.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:172
+      auto *SlotID = cast<ConstantAsMetadata>(MD->getOperand(1))->getValue();
+      auto *VoidPt =
+          new BitCastInst(AI, llvm::Type::getInt8PtrTy(C), "", InsertPoint);
----------------
```
- VoidPt
+ VoidPtr
```


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:176
+          Intrinsic::getDeclaration(M, Intrinsic::coro_frame_get),
+          {CoroBegin, VoidPt, IsPromise, SlotID}, "", InsertPoint);
+      auto *NewPtr = new BitCastInst(FrameGet, AI->getType(), "", InsertPoint);
----------------
We need to document the semantics for `coro.frame.get`


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:193
+                            F.getName() + ".ramp");
+    NewF->addFnAttr(Attribute::NoInline);
+    M->getFunctionList().push_back(NewF);
----------------
Noticed that this patch deletes `F.addFnAttr(CORO_PRESPLIT_ATTR, UNPREPARED_FOR_SPLIT);` below, is it conflicting with `D100282 `. I want to know if we still ned to add `Noinline` attribute once `D100282 ` checked in.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:218
+        II->replaceAllUsesWith(
+            llvm::ConstantInt::get(llvm::Type::getInt1Ty(C), 0));
+        break;
----------------
Why do we need to replace `coro.alloc` with 0 now?
Replace `coro.alloc` with 0 implies we should allocate the frame in the stack. I think we can't know how should we allocate the frame now.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:333
       CF->setArgOperand(0, CoroId);
+    splitRampFunction(F);
+  }
----------------
Should we give a another name for `splitRampFunction`? It may be surprising to see `split` in Coro-early pass instead of Coro-split pass.
BTW, how do you think about create the ramp function in the CodeGen process of frontend?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100415