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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
