ychen added inline comments.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:437-475
+void emitDynamicAlignedDealloc(CodeGenFunction &CGF,
+ llvm::BasicBlock *AlignedFreeBB,
+ llvm::CallInst *CoroFree) {
+ llvm::CallInst *Dealloc = nullptr;
+ for (llvm::User *U : CoroFree->users()) {
+ if (auto *CI = dyn_cast<llvm::CallInst>(U))
+ if (CI->getParent() == CGF.Builder.GetInsertBlock())
----------------
ChuanqiXu wrote:
> We don't need this in this patch.
Do you mean `// Match size_t argument with the one used during allocation.` or
the function `emitDynamicAlignedDealloc`? I think either is needed here. Could
you please elaborate?
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:555-558
+ explicit CallCoroDelete(Stmt *DeallocStmt, Stmt *AlignedDeallocStmt,
+ bool DynamicAlignedDealloc)
+ : Deallocate(DeallocStmt), AlignedDeallocate(AlignedDeallocStmt),
+ DynamicAlignedDealloc(DynamicAlignedDealloc) {}
----------------
ChuanqiXu wrote:
> Do we still need this change?
Nope
================
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;
----------------
ChuanqiXu wrote:
> ychen wrote:
> > ChuanqiXu wrote:
> > > 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.
> > I'm open to not calling `EmitBuiltinAlignTo`, which basically inline the
> > useful parts of `EmitBuiltinAlignTo`. The initial intention is code
> > sharing and easy readability. What's the benefit of not calling it?
> Reusing code is good. But my main concern is that the design for the
> interfaces. The current design smells bad to me. Another reason for implement
> it in place is I think it is not very complex and easy to understand.
>
> Another option I got is to implement `EmitBuitinAlign` in LLVM (someplace
> like `Local`), then the CodeGenFunction:: EmitBuitinAlign and this function
> could use it.
> Reusing code is good. But my main concern is that the design for the
> interfaces. The current design smells bad to me. Another reason for implement
> it in place is I think it is not very complex and easy to understand.
>
> Another option I got is to implement `EmitBuitinAlign` in LLVM (someplace
> like `Local`), then the CodeGenFunction:: EmitBuitinAlign and this function
> could use it.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1920-1921
+ llvm::Value *EmitBuiltinAlignTo(void *Args, const Expr *E, bool AlignUp);
+
public:
----------------
ChuanqiXu wrote:
> ychen wrote:
> > ChuanqiXu wrote:
> > > We shouldn't add this interface. The actual type for the first argument
> > > is BuiltinAlignArgs*, which defined in .cpp files. The signature is
> > > confusing.
> > This is a private function, supposedly only meaningful for the
> > implementation. In that situation do you think it's bad?
> It makes no sense to me that we can add interfaces casually if it is private.
> For the users of Clang/LLVM, it may be OK since they wouldn't look into the
> details. But for the developers, it matters. For example, I must be very
> confused when I see this signature. Why is the type of `Args` is void*?
> What's the type should I passed in? The smell is really bad.
> It makes no sense to me that we can add interfaces casually if it is private.
> For the users of Clang/LLVM, it may be OK since they wouldn't look into the
> details. But for the developers, it matters. For example, I must be very
> confused when I see this signature. Why is the type of `Args` is void*?
> What's the type should I passed in? The smell is really bad.
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
