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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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

> Here are the options I think the committee might take:
>
> 1. Always select an unaligned allocator and force implementors to dynamically align.  This seems unlikely to me.
> 2. Allow an aligned allocator to be selected.  The issue here is that we cannot know until coroutine splitting whether the frame has a new-extended alignment.  So there are some sub-options:
>
> 2a. Always use an aligned allocator if available, even if the frame ends up not being aligned.  I think this is unlikely.
> 2b. Use the correct allocator for the frame alignment, using the alignment inferred from the immediate coroutine body.  This would force implementations to avoid doing anything prior to coroutine splitting that would increase the frame's alignment beyond the derived limit.  This would be quite annoying for implementors, and it would have strange performance cliffs, but it's theoretically simple.
> 2c. Use the correct allocator for the frame alignment; only that allocator is formally ODR-used.  This would force implementations to not ODR-use the right allocator until coroutine lowering, which is not really reasonable; we should be able to dissuade the committee from picking this option.
> 2d. Use the correct allocator for the frame alignment; both allocators are (allowed to be) ODR-used, but only one would be dynamically used.  This is what would be necessary for the implementation I suggested above.  In reality there won't be any dynamic overhead because we should always be able to fold the branch after allocation.
>
> I think 2d is obvious enough that we could go ahead and implement it pending standardization.  There's still a question of what to do if the promise class only provides an unaligned `operator new`, but the only reasonable behavior is to dynamically align: unlike with `new` expressions, there's no way the promise class could be expected to know/satisfy the appropriate alignment requirement in general, so assuming alignment is not acceptable.  (Neither is rejecting the program if it doesn't provide both — this wouldn't be compatible with existing behavior.)
>
>> *(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?
>
> You're right that there might be space in the frame we could re-use.  I was thinking that it would be a shame to always add storage to the frame for the raw frame pointer, but maybe the contract of `llvm.coro.raw.frame.ptr.offset` could be that it's only meaningful if the frame has extended alignment.  Coroutine splitting would determine if any of the frame members was over-aligned and add a raw-pointer field if so. We'd be stuck allocating space in the frame even when allocation was elided, but stack space is cheap.
>
> It does need to be an offset instead of a type index, though; the frontend will be emitting a GEP and will not know the frame type.

Sounds good to me. Thanks. I'll go ahead with `llvm.coro.raw.frame.ptr.offset`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100739