ChuanqiXu added a comment. In D97915#2835147 <https://reviews.llvm.org/D97915#2835147>, @ychen wrote:
> In D97915#2832667 <https://reviews.llvm.org/D97915#2832667>, @ChuanqiXu wrote: > >> In D97915#2832446 <https://reviews.llvm.org/D97915#2832446>, @ychen wrote: >> >>> In D97915#2829581 <https://reviews.llvm.org/D97915#2829581>, @ChuanqiXu >>> wrote: >>> >>>> A remained question. >>>> >>>> - what's the semantics if user specified their allocation/deallocation >>>> functions? >>>> >>>> Previously, we discussed for the ::operator new and ::operator delete. But >>>> what would we do for user specified allocation/deallocation functions? >>>> It looks like we would treat them just like `::operator new`. And it makes >>>> sense at the first glance. But the problem comes from that we judge whether >>>> or not to over allocate a frame by this condition: >>>> >>>> coro.align > align of new >>>> >>>> But if the user uses their new, it makes no sense to use the `align of >>>> new` as the condition. On the other hand, if user specified their new >>>> function and the >>>> alignment requirement for their promise type, would it be better really >>>> that the compiler do the extra transformation? >>>> >>>> May be we need to discuss with other guys who are more familiar with the >>>> C++ standard to answer this. >>> >>> I think @rjmccall could answer these. My understanding is that >>> user-specified allocation/deallocation has the same semantics as their >>> standard library's counterparts. `align of new` (aka >>> __STDCPP_DEFAULT_NEW_ALIGNMENT__) should apply to both. >> >> Yeah, I just curious about the question and not sure about the answer yet. I >> agree with that it should be safe if we treat user-specified >> allocation/deallocation as ::operator new. Maybe I am a little bit of >> pedantry. I just not sure if the developer would be satisfied when they find >> we allocate padding space they didn't want when they offered a new/delete >> method. (Maybe I am too anxious). >> >> --- >> >> Another problem I find in this patch is that the introduction for `raw >> frame` makes the frame confusing. For example, the following codes is aim to >> calculate the size for the over allocated frame: >> >> %[[SIZE:.+]] = call i64 @llvm.coro.size.i64() >> %[[NEWSIZE:.+]] = add i64 %[[SIZE]], %[[PAD]] >> %[[MEM2:.+]] = call noalias nonnull i8* @_Znwm(i64 %[[NEWSIZE]]) >> >> It makes sense only if `llvm.coro.size` stands for the size of 'true frame' >> (I am not sure what's the meaning for raw frame now. Let me use 'true frame' >> temporarily). But the document now says that '@llvm.coro.size' returns the >> size of the frame. It's confusing >> I am not require to fix it by rename 'llvm.coro.size' or rephrase the >> document simply. I am thinking about the the semantics of coroutine >> intrinsics after we introduce the concept of 'raw frame'. > > These are great points. The semantics of some coroutine intrinsics needs a > little bit of tweaking otherwise they are confusing with the introduction of > `raw frame` (suggestions for alternative names are much appreciated) which I > defined in the updated patch (Coroutines.rst). Docs for `llvm.coro.size` > mentions `coroutine frame` which I've used verbatim in the docs for > `llvm.coro.raw.frame.ptr.*`. > Using your diagram below: `raw frame` is `| --- for align --- | --- true > frame --- |`. `Coroutine frame` is `| --- true frame --- |`. (BTW, > `llvm.coro.begin` docs are stale which I updated in the patch, please take a > look @ChuanqiXu @rjmccall @lxfind). Thanks for clarifying. Let's solve the semantics problem first. With the introduction about 'raw frame', I think it's necessary to introduce this concept in the section 'Switched-Resume Lowering' or even the section 'Introduction' in the document. Add a section to tell the terminology is satisfied too. Then why we defined both 'llvm.coro.raw.frame.ptr.offset' and 'llvm.coro.raw.frame.ptr.addr' together? It looks like refer to the same value finally. It looks like 'llvm.coro.raw.frame.ptr.offset' are trying to solve the problem about memory leak. But I think we could use llvm.coro.raw.frame.ptr.addr directly instead of traversing the frame (Maybe we need to add an intrinsic `llvm.coro.raw.size`). Then we can omit a field in the frame to save space. Then I am a little confused for the design again, since we would treat the value for CoroBegin as the address of coroutine frame in the past and it looks like to be the raw frame now. Let me reconsider if it is OK. ================ Comment at: llvm/docs/Coroutines.rst:963 + +The '``llvm.coro.align``' intrinsic returns the alignment of the coroutine frame +in bytes. ---------------- > '``llvm.coro.align``' `llvm.coro.align` ================ Comment at: llvm/docs/Coroutines.rst:1054 The second argument is a pointer to a block of memory where coroutine frame will be stored if it is allocated dynamically. This pointer is ignored ---------------- > coroutine frame From the implementation, it looks like `raw frame`. I am not sure if it is problematic now since CoroElide pass would convert the frame to an alloca. ================ Comment at: llvm/docs/Coroutines.rst:1087 The second argument is a pointer to the coroutine frame. This should be the same pointer that was returned by prior `coro.begin` call. ---------------- > the coroutine frame It should be the raw frame now, isn't it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97915/new/ https://reviews.llvm.org/D97915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
