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

[Xun Li via Phabricator via cfe-commits]

lxfind added a comment.

@ChuanqiXu Thank you for the detailed review! Really appreciate it.
I agree we should create a coroutine benchmark at some point, ideally some realistic production-code driven benchmark. We can work on that in the future. For this patch, it's probably not worth it to hide it behind an option, for two reasons: 1) it would be extremely complicated, 2) most parameters would end up on the frame anyway 3) this patch actually doesn't force parameters to be put on the frame. Before frame creation, all the parameters are put back to allocas, the current alloca analysis and optimization still applies to them. So some parameters may actually end up not put on the frame. So I wouldn't expect this to increase frame size in most cases.

I will add documentation latter once the we all agree on the high-level idea/direction of this patch.



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


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:193
+                            F.getName() + ".ramp");
+    NewF->addFnAttr(Attribute::NoInline);
+    M->getFunctionList().push_back(NewF);
----------------
ChuanqiXu wrote:
> 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.
Good question. For now they are somewhat redundant. We probably don't need to add NoInline here.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:218
+        II->replaceAllUsesWith(
+            llvm::ConstantInt::get(llvm::Type::getInt1Ty(C), 0));
+        break;
----------------
ChuanqiXu wrote:
> 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.
This is replacing it in the NewF (the cloned new ramp function). We only need to allocate the frame once, which will be done in the init function. So in the ramp function we can always skip it.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroEarly.cpp:333
       CF->setArgOperand(0, CoroId);
+    splitRampFunction(F);
+  }
----------------
ChuanqiXu wrote:
> 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?
I thought about doing it in CodeGen. But it's really complicated to split functions in CodeGen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100415