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