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

ychen added a comment. In D100739#2727974 , @ChuanqiXu wrote: > In D100739#2727808 , @ychen wrote: > >> In D100739#2718528 , @ChuanqiXu >> wrote: >> >>> In D100739#271851

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

ChuanqiXu added a comment. In D100739#2727808 , @ychen wrote: > In D100739#2718528 , @ChuanqiXu > wrote: > >> In D100739#2718514 , @ychen wrote: >> >>> Oh, right now C++

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

ychen added a comment. In D100739#2718528 , @ChuanqiXu wrote: > In D100739#2718514 , @ychen wrote: > >> Oh, right now C++ coroutine standard is written in the way that the aligned >> new is not searched by the fr

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

ychen abandoned this revision. ychen added a comment. Reopened D97915 to address the feedbacks. Close this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 _

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

ychen added a comment. In 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 ali

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

rjmccall added a comment. 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

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

ChuanqiXu added a comment. In D100739#2718514 , @ychen wrote: > Oh, right now C++ coroutine standard is written in the way that the aligned > new is not searched by the frontend. The limitation will be lifted in C++23 > (hopefully). I see. I am curious

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

ChuanqiXu added a comment. In D100739#2717582 , @rjmccall wrote: > 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 b

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

ychen added a comment. In D100739#2715964 , @ChuanqiXu wrote: > In D100739#2713579 , @ychen wrote: > >> In D100739#2711698 , @ChuanqiXu >> wrote: >> This is an alter

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

ychen added a comment. In D100739#2717582 , @rjmccall wrote: > In D100739#2717259 , @ychen wrote: > >> In D100739#2717227 , @rjmccall >> wrote: >> >>> Yes, if you can dyn

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

rjmccall added a comment. In D100739#2717259 , @ychen wrote: > In D100739#2717227 , @rjmccall > wrote: > >> Yes, if you can dynamically choose to use an aligned allocator, that's >> clearly just much better. > >

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

ychen added a comment. In 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

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

rjmccall added a comment. Yes, if you can dynamically choose to use an aligned allocator, that's clearly just much better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739

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

ChuanqiXu added a comment. In D100739#2713579 , @ychen wrote: > In D100739#2711698 , @ChuanqiXu > wrote: > >>> This is an alternative to D97915 which >>> missed proper deallocat

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

ychen added inline comments. Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:854 + // Save raw frame pointer to alloca + Value *Mem = Shape.CoroBegin->getMem(); + AllocaInst *FramePtrAddr = This seems to align with what this mem argument i

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

ychen added a comment. In D100739#2712268 , @lxfind wrote: >> Sorry for the confusion. I think either overaligned or under-aligned could >> be used here to describe the problem: either "Handle overaligned frame" or >> "Fix under-aligned frame". Since c+

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

ychen added a comment. In D100739#2711698 , @ChuanqiXu wrote: >> This is an alternative to D97915 which >> missed proper deallocation of the over-allocated frame. This patch handles >> both allocations and deallocations

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

lxfind added a comment. > Sorry for the confusion. I think either overaligned or under-aligned could be > used here to describe the problem: either "Handle overaligned frame" or "Fix > under-aligned frame". Since c++ spec defines the former but not the later > (https://en.cppreference.com/w/cpp

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

ChuanqiXu added a comment. > This is an alternative to D97915 which > missed proper deallocation of the over-allocated frame. This patch handles > both allocations and deallocations. If D97915 is not needed, we should abandon i

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

ychen updated this revision to Diff 339906. ychen added a comment. fix typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/test/CodeGenCoroutines/coro-al

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

ychen updated this revision to Diff 339902. ychen added a comment. Fix typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/test/CodeGenCoroutines/coro-al

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

ychen added a comment. In D100739#2711181 , @rjmccall wrote: > What is the purpose of the builtin? Where is it being used? Typically you > *can't* change the signature of a builtin because the builtin is itself a > language feature that's documented t

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

ychen added a comment. In D100739#2706973 , @lxfind wrote: > Thanks for working on this. > I am still having a bit hard time understanding the solution. > A few questions: > > 1. I assume this patch is to solve the problem where the promise object is > n

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

ychen updated this revision to Diff 339900. ychen edited the summary of this revision. ychen added a comment. - Address feebacks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 Files: clang/lib/CodeGen

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

rjmccall added a comment. What is the purpose of the builtin? Where is it being used? Typically you *can't* change the signature of a builtin because the builtin is itself a language feature that's documented to have a particular signature. If you've made a builtin purely for use in generate

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

lxfind added a comment. Thanks for working on this. I am still having a bit hard time understanding the solution. A few questions: 1. I assume this patch is to solve the problem where the promise object is not aligned according to its alignof annotation, right? The title/wording is a bit mislea

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

lxfind added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2689 - size_t __builtin_coro_size() + size_t __builtin_coro_size(bool alloc) void *__builtin_coro_frame() ChuanqiXu wrote: > ychen wrote: > > ChuanqiXu wrote: > > > ychen wrote: >

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

ChuanqiXu added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2689 - size_t __builtin_coro_size() + size_t __builtin_coro_size(bool alloc) void *__builtin_coro_frame() ychen wrote: > ChuanqiXu wrote: > > ychen wrote: > > > lxfind wrote: >

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

ychen added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2689 - size_t __builtin_coro_size() + size_t __builtin_coro_size(bool alloc) void *__builtin_coro_frame() ChuanqiXu wrote: > ychen wrote: > > lxfind wrote: > > > Do we need to chan

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

ChuanqiXu added a comment. There is something I am still confused about these two patch. Maybe I don't get the problem right. The example problem shows if user uses `alignas` to specify the alignment for promise_type, the actual alignment for the promise isn't correctly due to compiler didn't c

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

ychen added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2689 - size_t __builtin_coro_size() + size_t __builtin_coro_size(bool alloc) void *__builtin_coro_frame() lxfind wrote: > Do we need to change __builtin_coro_size? The argument wil

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

lxfind added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2689 - size_t __builtin_coro_size() + size_t __builtin_coro_size(bool alloc) void *__builtin_coro_frame() Do we need to change __builtin_coro_size? The argument will always be 1,

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

ChuanqiXu added a comment. In D100739#2700324 , @ychen wrote: > In D100739#2700273 , @ChuanqiXu > wrote: > >> I hadn't looked into the details. I would try to make it. >> But from my understanding to this problem

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

ychen added a comment. In D100739#2700273 , @ChuanqiXu wrote: > I hadn't looked into the details. I would try to make it. > But from my understanding to this problem, the correct solution should remain > the previous behavior if the end user doesn't spec

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

ChuanqiXu added a comment. I hadn't looked into the details. I would try to make it. But from my understanding to this problem, the correct solution should remain the previous behavior if the end user doesn't specify `alignas` for `promise_type`. I mean, it shouldn't make so many test cases fail

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

ychen updated this revision to Diff 338431. ychen added a comment. Fix comment. Ready for review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 Files: clang/docs/LanguageExtensions.rst clang/include

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

ychen updated this revision to Diff 338430. ychen added a comment. fix comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739 Files: clang/docs/LanguageExtensions.rst clang/include/clang/Basic/Builti

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

ychen created this revision. ychen added reviewers: rjmccall, lxfind, ChuanqiXu. Herald added a subscriber: hiraditya. ychen requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert. Herald added projects: clang, LLVM. This is an alternative to D97915