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

[Chuanqi Xu via Phabricator via cfe-commits]

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