[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

[Yuanfang Chen via Phabricator via llvm-commits]

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