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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

A remained question.

- what's the semantics if user specified their allocation/deallocation functions?

Previously, we discussed for the ::operator new and ::operator delete. But what would we do for user specified allocation/deallocation functions?
It looks like we would treat them just like `::operator new`. And it makes sense at the first glance. But the problem comes from that we judge whether
or not to over allocate a frame by this condition:

  coro.align > align of new

But if the user uses their new, it makes no sense to use the `align of new` as the condition. On the other hand, if user specified their new function and the 
alignment requirement for their promise type, would it be better really that the compiler do the extra transformation?

May be we need to discuss with other guys who are more familiar with the C++ standard to answer this.



================
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())
----------------
ychen wrote:
> 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? 
Sorry for that I misunderstand this function earlier.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:420
+
+void OverAllocateFrame(CodeGenFunction &CGF, llvm::CallInst *CI, bool IsAlloc) {
+  unsigned CoroSizeIdx = IsAlloc ? 0 : 1;
----------------
It looks like we'd better to add comment for this function.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:421
+void OverAllocateFrame(CodeGenFunction &CGF, llvm::CallInst *CI, bool IsAlloc) {
+  unsigned CoroSizeIdx = IsAlloc ? 0 : 1;
+  CGBuilderTy &Builder = CGF.Builder;
----------------
CoroSizeIdx should be zero all the time in this patch.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:431-433
+  auto *Diff = Builder.CreateNSWSub(AlignCall, AlignOfNewInt);
+  auto *NewCoroSize = Builder.CreateAdd(CI->getArgOperand(CoroSizeIdx), Diff);
+  CI->setArgOperand(CoroSizeIdx, NewCoroSize);
----------------
In other comments, I find 'size += align - NEW_ALIGN + sizeof(void*);'. But I don't find sizeof(void*) in this function.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:452-461
+  llvm::Function *RawFramePtrOffsetIntrin = CGF.CGM.getIntrinsic(
+      llvm::Intrinsic::coro_raw_frame_ptr_offset, CGF.Int32Ty);
+  auto *RawFramePtrOffset = CGF.Builder.CreateCall(RawFramePtrOffsetIntrin);
+  auto *FramePtrAddrStart =
+      CGF.Builder.CreateInBoundsGEP(CoroFree, {RawFramePtrOffset});
+  auto *FramePtrAddr = CGF.Builder.CreatePointerCast(
+      FramePtrAddrStart, CGF.Int8PtrTy->getPointerTo());
----------------
We allocate overaligned-frame like:
```
| --- for align --- | --- true frame --- |
```
And here we set the argument to the address of true frame. Then I wonder how about the memory for the `for align` part. Would we still free them correctly? Maybe I missed something.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:466-472
+  if (Dealloc->getNumArgOperands() > 1) {
+    // Size may only be the second argument of allocator call.
+    if (auto *CoroSize =
+            dyn_cast<llvm::IntrinsicInst>(Dealloc->getArgOperand(1)))
+      if (CoroSize->getIntrinsicID() == llvm::Intrinsic::coro_size)
+        OverAllocateFrame(CGF, Dealloc, /*IsAlloc*/ false);
+  }
----------------
We don't need to handle this condition in this patch.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:644
+  llvm::BasicBlock *RetOnFailureBB = nullptr;
+  llvm::BasicBlock *AlignAllocBB2 = nullptr;
 
----------------
It may be better to rename AlignAllocBB2 as AlignAllocBBCont or something similar.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:717
+    AlignAllocBB2 = createBasicBlock("coro.alloc.align2");
+    Builder.CreateCondBr(Cond, AlignAllocBB2, RetOnFailureBB);
+    EmitBlock(AlignAllocBB2);
----------------
It looks better to add an assert for RetOnFailureBB. I think it may be nullptr at the first glance.


================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:731-732
+                      {RawFramePtrAddr, getPointerAlign()});
+  AlignedAllocateCall = AlignedUpAddr;
+
   EmitBlock(InitBB);
----------------
We remove this assignment and use AlignedUpAddr directly in the following.


================
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;
----------------
ychen wrote:
> 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.
> 
> 
I guess you forgot to reply what you want to say.


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


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1133-1137
   // Create an entry for every spilled value.
   for (auto &S : FrameData.Spills) {
     FieldIDType Id = B.addField(S.first->getType(), None);
     FrameData.setFieldIndex(S.first, Id);
   }
----------------
Why we move this snippet to the front? Although it is not defined, the layout for the current frame would be:
```
| resume func addr | destroy func addr | promise | other things needed |
```
Move this to the front may break this.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1141-1144
+    // Add PromiseAlloca to Allocas list so that
+    // 1. updateLayoutIndex could update its index after
+    // `performOptimizedStructLayout`
+    // 2. it is processed in insertSpills.
----------------
We need to edit the comment too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97915