ChuanqiXu added a comment.
It looks not easy to review completely. After a rough review, the first suggest
is to remove the contents that should be in 'D102147
<https://reviews.llvm.org/D102147>', it makes the complex problem more complex
I think.
I would try to do more detailed review and test it if possible.
================
Comment at: clang/include/clang/AST/StmtCXX.h:331-335
+ ReturnValue, ///< Return value for thunk function: p.get_return_object().
+ ResultDecl, ///< Declaration holding the result of get_return_object.
+ ReturnStmt, ///< Return statement for the thunk function.
ReturnStmtOnAllocFailure, ///< Return statement if allocation failed.
FirstParamMove ///< First offset for move construction of parameter copies.
----------------
Minor issue: the intention of the comments should be the same.
================
Comment at: clang/include/clang/AST/StmtCXX.h:356-359
Expr *Allocate = nullptr;
Expr *Deallocate = nullptr;
+ Expr *AlignedAllocate = nullptr;
+ Expr *AlignedDeallocate = nullptr;
----------------
ChuanqiXu wrote:
> ychen wrote:
> > ChuanqiXu wrote:
> > > ychen wrote:
> > > > ChuanqiXu wrote:
> > > > > Can't we merge these?
> > > > I'm not sure about the "merge" here. Could you be more explicit?
> > > Sorry. I mean if we can merge `Allocate` with `AlignedAllocate` and merge
> > > `Deallocate` with `AlignedDeallocate`. Since from the implementation, it
> > > looks like the value of `Allocate` and `AlignedAllocate ` (so as
> > > `Deallocate` and `AlignedDeallocate`) are the same.
> > Oh, this is to set the path for D102147 where `Allocate` and
> > `AlignedAllocate` could be different. If I do this in D102147, it will also
> > touch the `CGCoroutine.cpp` which I'm trying to avoid` since it is intended
> > to be a Sema only patch.
> Yeah, this is the key different point between us. I think that `D102147`
> could and should to touch the CodeGen part.
As we discussed before, I prefer to merge `Allocate` and `AlignedAllocate`
(also Deallocate and AlignedDeallocate ) in this patch. It looks weird the are
the same in one commit.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:420
+
+void overAllocateFrame(CodeGenFunction &CGF, llvm::CallInst *CI, bool IsAlloc)
{
+ unsigned CoroSizeIdx = IsAlloc ? 0 : 1;
----------------
We should capitalize it as 'OverAllocateFrame'
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:701-706
+ if (HasAlignArg) {
+ if (S.getReturnStmtOnAllocFailure()) {
+ auto *Cond = Builder.CreateICmpNE(AlignedAllocateCall, NullPtr);
+ Builder.CreateCondBr(Cond, InitBB, RetOnFailureBB);
+ }
+ } else {
----------------
`if (HasAlignArg)` should be the content of the next patch 'D102147', right? I
don't think they should come here.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:743-744
+ CGM.getIntrinsic(llvm::Intrinsic::coro_align, SizeTy));
+ auto *AlignedUpAddr = EmitBuiltinAlignTo(AlignedAllocateCall, CoroAlign,
+ S.getAlignedAllocate(), true);
+ // rawFrame = frame;
----------------
Maybe we could calculate it in place instead of trying to call a function which
is not designed for llvm::value*. It looks like the calculation isn't too
complex.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:753
+ }
+
EmitBlock(InitBB);
----------------
Does here miss a branch to InitBB?
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1920-1921
+ llvm::Value *EmitBuiltinAlignTo(void *Args, const Expr *E, bool AlignUp);
+
public:
----------------
We shouldn't add this interface. The actual type for the first argument is
BuiltinAlignArgs*, which defined in .cpp files. The signature is confusing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97915/new/
https://reviews.llvm.org/D97915
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
