[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:743-744
+        CGM.getIntrinsic(llvm::Intrinsic::coro_align, SizeTy));
+    auto *AlignedUpAddr = EmitBuiltinAlignTo(AlignedAllocateCall, CoroAlign,
+                                             S.getAlignedAllocate(), true);
+    // rawFrame = frame;
----------------
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?


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:753
+  }
+
   EmitBlock(InitBB);
----------------
ChuanqiXu wrote:
> Does here miss a branch to InitBB?
`EmitBlock` would handle the case.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:1920-1921
 
+  llvm::Value *EmitBuiltinAlignTo(void *Args, const Expr *E, bool AlignUp);
+
 public:
----------------
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? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97915/new/

https://reviews.llvm.org/D97915