[PATCH] D89768: [Coroutine] Properly determine whether an alloca should live on the frame

Tue Oct 27 23:36:32 PDT 2020

junparser added inline comments.

================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:921
+      for (auto *BB2 : UserBBs)
+        if (Checker.hasPathCrossingSuspendPoint(BB1, BB2))
+          return true;
----------------
lxfind wrote:
> junparser wrote:
> > lxfind wrote:
> > > lxfind wrote:
> > > > junparser wrote:
> > > > > junparser wrote:
> > > > > > lxfind wrote:
> > > > > > > junparser wrote:
> > > > > > > > lxfind wrote:
> > > > > > > > > junparser wrote:
> > > > > > > > > > Still need call Checker.hasPathCrossingSuspendPoint(AllocaDef,  Pointeruser)
> > > > > > > > > We cannot do that because the whole purpose of this rewrite is to check if uses of the alloca belong to different suspend regions, which causes the spill. 
> > > > > > > > > I thought about this a bit more, and my conclusion is that although the API is not intended to be used this way, it actually works here.
> > > > > > > > > The goal for this loop is to check whether an alloca needs to live across a suspension. If an alloca needs to live across a suspension, it means there exists a coro.suspend such that a user BB happens before it and a user BB happens after it, and these two BBs must dominate each other through coro.susend. Hence one of the user BB pairs must return true on the hasPathCrossingSuspendPoint check. On the other hand, for BBs that don't even dominate each other, hasPathCrossingSuspendPoint will return false anyway and won't affect the algorithm.
> > > > > > > > let's say :
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > void foo () {
> > > > > > > >   alloca a;
> > > > > > > >   if (cond)
> > > > > > > >     {
> > > > > > > >       co_await
> > > > > > > >       use1 a;
> > > > > > > >     }else
> > > > > > > >       use2 a;
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > in such case, hasPathCrossingSuspendPoint(use1, use2) return false which means a is not keep on frame, this is wrong.
> > > > > > > In this case, "a" is just defined but not used at all before the co_await, so it would be correct to not keep it on frame, right? Could you explain why in this case "a" needs to be on the frame?
> > > > > > > We cannot do that because the whole purpose of this rewrite is to check if uses of the alloca belong to different suspend regions, which causes the spill. 
> > > > > > > I thought about this a bit more, and my conclusion is that although the API is not intended to be used this way, it actually works here.
> > > > > > > The goal for this loop is to check whether an alloca needs to live across a suspension. If an alloca needs to live across a suspension, it means there exists a coro.suspend such that a user BB happens before it and a user BB happens after it, and these two BBs must dominate each other through coro.susend. Hence one of the user BB pairs must return true on the hasPathCrossingSuspendPoint check. On the other hand, for BBs that don't even dominate each other, hasPathCrossingSuspendPoint will return false anyway and won't affect the algorithm.
> > > > > > 
> > > > > >  I do not understand why there is a user bb happends before coro.suspend?
> > > > > It may used before the co_await,  as long as initial_suspend never suspend and cond is false. This pattern can also be put in a loop. we do not know whether part will be executed.
> > > > If an alloca needs to live across a suspension, there must exists two uses of the alloca (or through an alias) that one happens before a coro.suspend and one happens after that same coro.suspend. If no two uses of alloca can cross a suspend, the alloca does not ever need to live on the frame. Does this part sound right?
> > > > 
> > > > Next we need to prove that if there exists such a pair of use, one of them will return true on hasPathCrossingSuspendPoint.
> > > > It's obvious that the alloca use after coro.suspend must be dominated by coro.suspend. The alloca happens before coro.suspend is a bit trickier to think about. In the simpler case, if the alloca happens before coro.suspend also dominates coro.suspend, then we have a pair of user BBs that would return true on the check, because they dominate each other through coro.suspend. If the alloca happens before coro.suspend does not dominate coro.suspend, and yet it's meaningfully used, it would either escape at some point, or it will be propagated into a PHINode eventually and that PHINode will dominate coro.suspend. Since we also track PHINode in the alias analysis, we will eventually find this pair that dominates each other.
> > > > It may used before the co_await, as long as initial_suspend never suspend and cond is false. This pattern can also be put in a loop. we do not know whether part will be executed.
> > > 
> > > In your code example, "a" is not used before co_await, neither initialized. So it doesn't matter if it lives on the frame or not. If it is used, then there will exists a user-pair that crosses suspensions.
> > > Could you explain what could go wrong in your example above if "a" is not kept in the frame?
> > > 
> > > If an alloca needs to live across a suspension, there must exists two uses of the alloca (or through an alias) that one happens before a coro.suspend and one happens after that same coro.suspend. If no two uses of alloca can cross a suspend, the alloca does not ever need to live on the frame. Does this part sound right?
> > > 
> > There is no before/after relation. as long as two uses used in different suspend region.  the case can be changed to 
> > 
> > ```
> > void foo () {
> >   alloca a;
> >   if (cond)
> >     {
> >       co_await
> >       use1 a;
> >     }else{
> >       co_await
> >       use2 a;
> >     }
> > }
> > ```
> > > Next we need to prove that if there exists such a pair of use, one of them will return true on hasPathCrossingSuspendPoint.
> > > It's obvious that the alloca use after coro.suspend must be dominated by coro.suspend. The alloca happens before coro.suspend is a bit trickier to think about. In the simpler case, if the alloca happens before coro.suspend also dominates coro.suspend, then we have a pair of user BBs that would return true on the check, because they dominate each other through coro.suspend. If the alloca happens before coro.suspend does not dominate coro.suspend, and yet it's meaningfully used, it would either escape at some point, or it will be propagated into a PHINode eventually and that PHINode will dominate coro.suspend. Since we also track PHINode in the alias analysis, we will eventually find this pair that dominates each other.
> > 
> > 
> Could you explain what could go wrong in this example if "a" is not on the frame?
> The "alloca" instruction simply defines a value, but nothing else.
> So your example code
> ```
> void foo () {
>   alloca a;
>   if (cond)
>     {
>       co_await
>       use1 a;
>     }else{
>       co_await
>       use2 a;
>     }
> }
> ```
> behaves exactly as
> ```
> void foo () {
>   if (cond)
>     {
>       co_await
>       alloca a1
>       use1 a1;
>     }else{
>       co_await
>       alloca a2
>       use2 a2;
>     }
> }
> ```
hmm... get your point.  make sense to me. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89768