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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

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

> In D88872#2323145 <https://reviews.llvm.org/D88872#2323145>, @ChuanqiXu wrote:
>
>> 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?
>
> It's harder to generate an example from source code (it has to be quite complicated, I have some production code, but not small enough to share). But it's easy to see from IR examples.
> Consider the following IR (you can run it through `opt --coro-split`:
>
>   define i8* @f(i1 %n) "coroutine.presplit"="1" {
>   entry:
>     %a = alloca i32, align 8
>     %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
>     %size = call i32 @llvm.coro.size.i32()
>     %alloc = call i8* @malloc(i32 %size)
>     %hdl = call i8* @llvm.coro.begin(token %id, i8* %alloc)
>   
>     %flag = call i1 @check()
>     br i1 %flag, label %flag_true, label %flag_false
>   
>   flag_true:
>     %b = bitcast i32* %a to i8*
>     br label %merge
>   
>   flag_false:
>     %c = bitcast i32* %a to i8*
>     br label %merge
>   
>   merge:
>     %d = phi i8* [ %b, %flag_true ], [ %c, %flag_false ]
>     %sp1 = call i8 @llvm.coro.suspend(token none, i1 false)
>     switch i8 %sp1, label %suspend [i8 0, label %resume
>                                     i8 1, label %cleanup]
>   resume:
>     call void @print(i8* %d)
>     br label %cleanup
>   
>   cleanup:
>     %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
>     call void @free(i8* %mem)
>     br label %suspend
>   suspend:
>     call i1 @llvm.coro.end(i8* %hdl, i1 0)
>     ret i8* %hdl
>   }
>   
>   declare i8* @llvm.coro.free(token, i8*)
>   declare i32 @llvm.coro.size.i32()
>   declare i8  @llvm.coro.suspend(token, i1)
>   declare void @llvm.coro.resume(i8*)
>   declare void @llvm.coro.destroy(i8*)
>   
>   declare token @llvm.coro.id(i32, i8*, i8*, i8*)
>   declare i1 @llvm.coro.alloc(token)
>   declare i8* @llvm.coro.begin(token, i8*)
>   declare i1 @llvm.coro.end(i8*, i1)
>   
>   declare noalias i8* @malloc(i32)
>   declare i1 @check()
>   declare void @print(i8*)
>   declare void @free(i8*)
>
> Notice that `%a`'s alias is stored in a PHI, which is used after suspend. When you run coro-split on it, the current implementation will think that only the PHI needs to be spilled, while %a can stay on stack.
> So in the generated IR, %a will still be an alloca, but a pointer to it will be store to the frame! The generated IR looks like this:
>
>   define i8* @f(i1 %n) {
>   entry:
>     %a = alloca i32, align 8
>     ...
>     %b = bitcast i32* %a to i8*
>     %d.spill.addr = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 2
>     store i8* %b, i8** %d.spill.addr, align 8
>     ...
>   }
>
> This is just one example, but you can also escape it by storing the address, or call a function.

Good example! I got it. This patch looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88872