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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen marked an inline comment as done.
ychen 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;
----------------
ChuanqiXu wrote:
> Can't we merge these?
I'm not sure about the "merge" here. Could you be more explicit?


================
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())
----------------
ChuanqiXu wrote:
> 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.
It handles both aligned and normal new/delete.


================
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;
----------------
ChuanqiXu wrote:
> 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?
Agree that `LastCoroFreeUsedForDealloc` is a bit confusing. It makes sure deallocation and aligned deallocation share one `coro.free`. Otherwise, AFAIK, there would be two `coro.free` get codegen'd.
```
%mem = llvm.coro.free()
br i1 <overalign> , label <aligend-dealloc>, label <dealloc>

aligend-dealloc:
    use %mem

dealloc:
    use %mem
```


> what's the reason we need to write emit(deallocate) twice?
John wrote a code snippet here: https://reviews.llvm.org/D100739#2717582. I think it would be helpful to look at the changed tests below to see the patterns.

Basically, for allocation, it looks like below; for deallocation, it would be similar.
```
void *rawFrame =nullptr;
...
if (llvm.coro.alloc()) {
  size_t size = llvm.coro.size(), align = llvm.coro.align();
  if (align > NEW_ALIGN) {
#if <an allocation function without std::align_val_t argument is selected by Sema>
    size += align - NEW_ALIGN + sizeof(void*);
    frame = operator new(size);
    rawFrame = frame;
    frame = (frame + align - 1) & ~(align - 1);
#else
    // If an aligned allocation function is selected.
    frame = operator new(size, align);
#endif
  } else {
    frame = operator new(size);
  }
}
```
The true branch of the #if directive is equivalent to "coro.alloc.align" block (and "coro.alloc.align2" if `get_return_object_on_allocation_failure` is defined), the false branch is equivalent to "coro.alloc" block.
The above pattern handles both aligned/normal allocation/deallocation so it is independent of D102147.


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


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:702-704
+  if (!HasAlignArg)
+    overAllocateFrame(*this, cast<llvm::CallInst>(AlignedAllocateCall),
+                      /*IsAlloc*/ true);
----------------
ChuanqiXu wrote:
> 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.
will do.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:878
+    if (CurCoro.Data && CurCoro.Data->LastCoroFreeUsedForDealloc)
+      return RValue::get(CurCoro.Data->LastCoroFree);
+
----------------
ChuanqiXu wrote:
> Is it possible that it would return a nullptr value?
Not that I know of. Because there is an early return 
```
    if (!CoroFree) {
      CGF.CGM.Error(Deallocate->getBeginLoc(),
                    "Deallocation expressoin does not refer to coro.free");
      return;
    }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97915