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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/AST/StmtCXX.h:356-359
     Expr *Allocate = nullptr;
     Expr *Deallocate = nullptr;
+    Expr *AlignedAllocate = nullptr;
+    Expr *AlignedDeallocate = nullptr;
----------------
Can't we merge these?


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:445-483
+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())
----------------
This code would only work if we use `::operator new(size_t, align_val_t)`, which is implemented in another patch. I would suggest to move this into that one.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:528-547
     CGF.EmitBlock(FreeBB);
     CGF.EmitStmt(Deallocate);
-
-    auto *AfterFreeBB = CGF.createBasicBlock("after.coro.free");
-    CGF.EmitBlock(AfterFreeBB);
+    CGF.Builder.CreateBr(AfterFreeBB);
 
     // We should have captured coro.free from the emission of deallocate.
     auto *CoroFree = CGF.CurCoro.Data->LastCoroFree;
+    CGF.CurCoro.Data->LastCoroFreeUsedForDealloc = true;
----------------
It looks like it would emit a `deallocate` first, and emit an `alignedDeallocate`, which is very odd. Although I can find that the second `deallocate` wouldn't be emitted due to the check `LastCoroFreeUsedForDealloc`, it is still very odd to me. If the second `deallocate` wouldn't come up all the way, what's the reason we need to write `emit(deallocate)` twice?


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:700
+  auto *AlignedAllocateCall = EmitScalarExpr(S.getAlignedAllocate());
+  bool HasAlignArg = hasAlignArg(cast<llvm::CallInst>(AlignedAllocateCall));
+
----------------
Since `hasAlignArg` is called only once, I suggested to make it a lambda here which would make the code more easy to read.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:702-704
+  if (!HasAlignArg)
+    overAllocateFrame(*this, cast<llvm::CallInst>(AlignedAllocateCall),
+                      /*IsAlloc*/ true);
----------------
I recommend to add a detailed comment here to tell the story why we need to over allocate the frame. It is really hard to understand for people who are new to this code. Otherwise, I think they need to use `git blame` to find the commit id and this review page to figure the reasons out.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:706-728
+  if (auto *RetOnAllocFailure = S.getReturnStmtOnAllocFailure()) {
+    auto *Cond = Builder.CreateICmpNE(AlignedAllocateCall, NullPtr);
+    if (HasAlignArg) {
+      Builder.CreateCondBr(Cond, InitBB, RetOnFailureBB);
+    } else {
+      AlignAllocBB2 = createBasicBlock("coro.alloc.align2");
+      Builder.CreateCondBr(Cond, AlignAllocBB2, RetOnFailureBB);
----------------
It may be better to organize it as:
```
if (!HasAlignArg) {
   if (auto *RetOnAllocFailure = S.getReturnStmtOnAllocFailure()) {
       auto *Cond = Builder.CreateICmpNE(AlignedAllocateCall, NullPtr);
       AlignAllocBB2 = createBasicBlock("coro.alloc.align2");
       Builder.CreateCondBr(Cond, AlignAllocBB2, RetOnFailureBB);
       EmitBlock(AlignAllocBB2);
   }
   auto *CoroAlign = Builder.CreateCall(
        CGM.getIntrinsic(llvm::Intrinsic::coro_align, SizeTy));
   ...
}
```


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:878
+    if (CurCoro.Data && CurCoro.Data->LastCoroFreeUsedForDealloc)
+      return RValue::get(CurCoro.Data->LastCoroFree);
+
----------------
Is it possible that it would return a nullptr value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97915