[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

Thu Jun 24 00:15:28 PDT 2021

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