[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D100739#2717582 <https://reviews.llvm.org/D100739#2717582>, @rjmccall wrote:

> In D100739#2717259 <https://reviews.llvm.org/D100739#2717259>, @ychen wrote:
>
>> In D100739#2717227 <https://reviews.llvm.org/D100739#2717227>, @rjmccall wrote:
>>
>>> Yes, if you can dynamically choose to use an aligned allocator, that's clearly just much better.
>>
>> Right now:
>>
>> Intrinsic::coro_size_aligned :   overaligned frame: over-allocate, adjust start address;    non-overaligned frame: no-op
>> Intrinsic::coro_size :                overaligned frame: no-op;                                                 non-overaligned frame: no-op
>>
>> Do you mean to remove `Intrinsic::coro_size_aligned` and make 
>> Intrinsic::coro_size :                overaligned frame: over-allocate, adjust start address;    non-overaligned frame: no-op
>>
>> that makes sense to me. Just want to confirm first.
>
> That's not really what I meant, no.  I meant it would be better to find a way to use an allocator that promises to return a well-aligned value when possible.  We've talked about this before; that will require the C++ committee to update the design.
>
> I think the cleanest design for coro_size/align would be that they reflect the unadjusted requirements, and the frontend is expected to emit code which satisfies those requirements.  In the absence of an aligned allocator, that means generating code like:
>
>   if (llvm.coro.alloc()) {
>     size_t size = llvm.coro.size(), align = llvm.coro.align();
>     if (align > NEW_ALIGN)
>       size += align - NEW_ALIGN + sizeof(void*);
>     frame = operator new(size);
>     if (align > NEW_ALIGN) {
>       auto rawFrame = frame;
>       frame = (frame + align - 1) & ~(align - 1);
>       *(void**) (frame + size) = rawFrame;
>     }
>   }
>
> and then on the deallocation side:
>
>   if (llvm.coro.alloc()) {
>     size_t size = llvm.coro.size(), align = llvm.coro.align();
>     if (align > NEW_ALIGN)
>       frame = *(void**) (frame + size);
>     operator delete(frame);
>   }
>
> That's all quite annoying, but it does extend quite nicely to cover the presence of an aligned allocator when the committee gets around to ratifying that this is what should happen:
>
>   if (llvm.coro.alloc()) {
>     size_t size = llvm.coro.size(), align = llvm.coro.align();
>     if (align > NEW_ALIGN)
>       frame = operator new(std::align_val_t(align), size);
>     else
>       frame = operator new(size);
>   }
>
> and then on the deallocation side:
>
>   if (llvm.coro.alloc()) {
>     size_t size = llvm.coro.size(), align = llvm.coro.align();
>     if (align > NEW_ALIGN)
>       operator delete(frame, std::align_val_t(align));
>     else
>       operator delete(frame);
>   }

`*(void**) (frame + size) = rawFrame;` this means we always need the extra space to store the raw frame ptr. If either doing what the patch is currently doing or add another intrinsic say "llvm.coro.raw.frame.ptr.index" to do `*(void**) (frame + llvm.coro.raw.frame.ptr.index()) = rawFrame;`, it is likely that the extra pointer could reuse some existing paddings in the frame.  There is an example of this in https://reviews.llvm.org/P8260. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100739