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

[JunMa via Phabricator via llvm-commits]

junparser added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:921
+      for (auto *BB2 : UserBBs)
+        if (Checker.hasPathCrossingSuspendPoint(BB1, BB2))
+          return true;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89768