ychen added inline comments.
================
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:
> > 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.
> I think I know where the confusion comes from. `AlignedDeallocate` is not
> guaranteed to be an aligned allocator. In this patch in `SemaCoroutine.cpp`,
> it is set to `Deallocate` in which case we always dynamically adjust frame
> alignment. Once D102147 is landed. `AlignedDeallocate` may or may not be an
> aligned allocator.
>
> > 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).
>
> I was worried about the size of the patch if this is merged with D102145 but
> if that is preferred by more than one reviewer, I'll go ahead and do that.
> D102145 is pretty self-contained in that it does not contain clients of the
> added intrinsics but the introduced test should cover the expected intrinsic
> lowering.
>
Naming is hard. I had a hard time figuring out a better name.
`AlignedDeallocate`/`AlignedAllocate` is intended to refer to
allocator/deallocator used for handling overaligned frame. Not that they are
referring to allocator/deallocator with std::align_val_t argument.
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
