[PATCH] D87293: [GVN] Fix undef incoming value for phi node when new loop exit block created.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2758
+    SmallVector<BasicBlock *, 4> Candidates;
     for (BasicBlock *P : Preds) {
       if (!DeadBlocks.count(P))
----------------
`for (BasicBlock *P : predecessors(B)) {`

delete Preds.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2751
     }
 
+    for (BasicBlock *C : Candidates) {
----------------
wwei wrote:
> asbirlea wrote:
> > wwei wrote:
> > > asbirlea wrote:
> > > > Could you please explain in more details the mechanics here? 
> > > > When added into Candidates, the llvm::any_of condition is true. What makes the condition become false below?
> > > > What's the purpose of the intersection of non-dead succs and preds?
> > > I have updated the code, and some comments to descibe the details for it.
> > > 
> > > ```
> > >     //     - - - - - - - -
> > >     //    |    ... ...    |
> > >     //    |[D1]           |   =>  A simplified loop<L>
> > >     //    | |  \          |
> > >     //    | |   [D2] [B1] |
> > >     //     -|- - |- - |- -
> > >     //      |    |    |
> > >     //      \    |    /
> > >     //       \   |   /
> > >     //        [ DF1 ]        => exit block for loop<L>
> > >     //
> > >     // Consider the above scenario, there're two dead blocks [D1] + [D2], and
> > > // a live block[B1]([D1][D2][B1] in one loop). [DF1] will a common dominance
> > >     // frontier block for [D1] and [D2], so Candidates will contain two elements
> > >     // [D1, D2]. [D1] has two succs: [D2] and [DF1], for [D1],first llvm::any_of
> > >     // will return true, so splitCriticalEdges will be called.
> > >     //
> > >     //     - - - - - - - -
> > >     //    |    ... ...    |
> > >     //    |[D1]           |   =>  A simplified loop<L>
> > >     //    | |  \          |
> > >     //    | |   [D2] [B1] |
> > >     //     -|- - |- - |- -
> > >     //      |    |    |
> > >     //      |    \    /
> > >     //     [D3]   [DF2]      => New Exit block[DF2] for loop<L>
> > >     //       \      |
> > >     //        [ DF1 ]
> > >     //
> > >     // After splitCriticalEdges, a new crit_edge block[D3] will be created, and
> > >     // it is added into dead blocks. Also, a new loop exit block[DF2] is created
> > >     // to preserve LoopSimplify. After that, llvm::any_of will return false for
> > >     // [D2], and we need to add [DF2] as a new dominance frontier block for [D2]
> > >     // since it has a pred edge to non-dead block[B1], and then, we need to
> > >     // update the undef incoming value for phi in [DF2].
> > > ```
> > > 
> > > About the purpose of the intersection of non-dead succs and preds, we should consider another case like below:
> > > 
> > > ```
> > > //     - - - - - - - -
> > > //    |    ... ...    |
> > > //    |    ... ...    |  =>  A simplified loop<L>
> > > //    | [  D1 ]  [B1] |
> > > //     -|- |- |- -|- - 
> > > //      |  |  |   |
> > > //      |  |  |   |
> > > //      \  |  |   |
> > > //       \ |  /  /
> > > //        [ DF1 ]        => exit block for loop<L>
> > > //
> > > // After split edeges,
> > > //     - - - - - - - -
> > > //    |    ... ...    |
> > > //    |    ... ...    |  =>  A simplified loop<L>
> > > //    | [  D1  ]  [B1]|
> > > //     -|-  |- |- -|-- 
> > > //      |   |  |   |
> > > //      |   |  \   /
> > > //     [D2][D3][DF2]     => New Exit block[DF2] for loop<L>
> > > //       \  |    /
> > > //        [ DF1 ]
> > > //
> > > //    switch-case: [  D1  ] will have multi edges to  [ DF1 ], after split,  [D2][D3] are new created,
> > > //    both will be dead blocks, so we need to use  the intersection of non-dead succs and preds to get [DF2]
> > > ```
> > Thank you for adding the example in code, that's very helpful!
> > 
> > I may be missing some details in the last example, but it seems like the non-dead predecessors for B (the IDF block) should cover the new blocks? (`[D2]`and `[D3]` should be in DeadBlocks following the split). 
> yeah, the non-dead predecessors for B should cover the new blocks, there're some mistakes for the diagram after split edeges, the right one should be:
> ```
> // After split edeges,
> //     - - - - - - - -
> //    |    ... ...    |
> //    |    ... ...    |  =>  A simplified loop<L>
> //    | [  D1  ]  [B1]|
> //     -|-  |- |- -|-- 
> //      |   |  |   |
> //      |   \  |  /
> //     [D2]  [DF2]       => New Exit block[DF2] for loop<L>
> //       \     |
> //        [ DF1 ]
> ```
>  [D2]  and  [DF2] are both new created blocks after  split edeges,[D2] will be a dead block while  [DF2] will be a new IDF block
I didn't review further because I didn't see the change in code.  Doesn't this mean just the predecessors is enough, vs intersection of succs and preds?


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

https://reviews.llvm.org/D87293