ChuanqiXu added a comment.
It misses the part in llvm now.
================
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())
----------------
We don't need this in this patch.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:477
+
+void emitCheckAlignBasicBlock(CodeGenFunction &CGF,
+ llvm::BasicBlock *CheckAlignBB,
----------------
Capitalize `EmitCheckAlignBasicBlock`
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:555-558
+ explicit CallCoroDelete(Stmt *DeallocStmt, Stmt *AlignedDeallocStmt,
+ bool DynamicAlignedDealloc)
+ : Deallocate(DeallocStmt), AlignedDeallocate(AlignedDeallocStmt),
+ DynamicAlignedDealloc(DynamicAlignedDealloc) {}
----------------
Do we still need this change?
================
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;
----------------
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.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1920-1921
+ llvm::Value *EmitBuiltinAlignTo(void *Args, const Expr *E, bool AlignUp);
+
public:
----------------
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.
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
