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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
