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;
----------------
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.
================
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;
----------------
ychen wrote:
> 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.
Thanks. I get the reason why I am thinking the code isn't natural. Since I
think `::operator new(size_t, align_val_t)` shouldn't come up in this patch
which should be available after D102147 applies. Here you said this patch is
independent with D102147, I believe this patch could work without D102147. But
it contains the codes which would work only if we applies the successor patch,
so I think it is dependent on D102147.
The ideally relationship for me is to merge `D102145 ` into this one (Otherwise
it is weird for me that `D102145` only introduces some intrinsics which
wouldn't be used actually). Then this patch should handle the alignment for
variables in coroutine frame without introducing `::new(size_t, align_val_t)`.
Then the final patch could do the job that searching and generating code for
`::new(size_t, align_val_t)`.
Maybe it is a little bit hard to rebase again and again. But I think it is
better.
================
Comment at: clang/lib/CodeGen/CGCoroutine.cpp:878
+ if (CurCoro.Data && CurCoro.Data->LastCoroFreeUsedForDealloc)
+ return RValue::get(CurCoro.Data->LastCoroFree);
+
----------------
ychen wrote:
> 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;
> }
> ```
Do you think it is better to merge this check here?
```
if (CurCoro.Data && CurCoro.Data->LastCoroFreeUsedForDealloc) {
if (!CoroFree) {
CGF.CGM.Error(Deallocate->getBeginLoc(),
"Deallocation expressoin does not refer to coro.free");
return something;
}
return RValue::get(CurCoro.Data->LastCoroFree);
}
```
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
