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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100739