[PATCH] D88872: [Coroutines] Refactor/Rewrite Spill and Alloca processing

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D88872#2321902 <https://reviews.llvm.org/D88872#2321902>, @lxfind wrote:

> In D88872#2320823 <https://reviews.llvm.org/D88872#2320823>, @ChuanqiXu wrote:
>
>> Thanks for your patch! It mentions some bugs about allocas we need to handle in the future.
>>
>> For this patch, I'm little confusing about why we need to separate alloca from spills. In my mind, a `spill` means something we need to put in the frame. And an alloca which would be in the frame is naturally a spill.  
>> I think the patch benefits from replacing
>>
>>   using SpillInfo = SmallVector<Spill, 8>;
>>
>> to
>>
>>   using SpillInfo = SmallMapVector<Value *, SmallVector<Instruction *, 2>, 8>;
>
> Thanks for taking a look at the patch. If you look at the implementation, the handling of "Spills" and always different from the handling of allocas. They do share the same concept that they need to go to the frame (which is why they both belong to FrameDataInfo).
> The primary reason to separate them (and hence set up the code for future fixes), is this one primary difference between them: A Spill is a direct def-use relationship that crosses suspension points; while an alloca may not be exposed to a direct def-use relationship that crosses suspension points but still need to go to the frame. The condition for them to go to the frame is fundamentally different.

I agree with we can benefit from separating alloca from spills. At least we don't need extract allocas from spills by a redundant loop any more. After we separate allocas from spills, the name `spills` seems a little strange. But I think it doesn't really matter.

Here is a question about the bug mentioned in summary. I write a simple code like this:

  void consuming(int* pa);
  
  task escape_alloca() {
    int a;
    consuming(&a);
    co_await something();
  }

But clang would still put a in the frame in O0 or O2 <https://reviews.llvm.org/owners/package/2/>. I guess it is because the def of `a` and the use of `a` is cross initial_suspend (in O0 mode) or the lifetime markers is cross the `co_await something` point. Is there anything wrong? Or what is the example about the bug?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88872