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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
