[PATCH] D46422: [LCSSA] Iteratively remove unused PHI nodes in formLCSSAForInstructions

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D46422#1088719, @bjope wrote:

> In https://reviews.llvm.org/D46422#1088261, @mzolotukhin wrote:
>
> > > Is it guaranteed that a PHI node, without uses, being inserted in PHIsToRemove only can be used by later created PHI nodes
> >
> > PHI-nodes are only inserted when we call SSAUpdater.RewriteUse. I did not expected that their use lists change afterwards, so frankly, I'm surprised that inserted PHIs get more uses on later iterations - that's why I'm asking for a simpler test that could help to understand how and why this happens.
> >
> > > Then it might be enough to iterate in reverse insertion order...
> >
> > Why do we need to reverse the order?
>
>
> As seen in my earlier post we remove the following PHI nodes when using my patch:
>
>   %value.lcssa2 = phi i16 [ %value3.lcssa, %exit1.pred ], [ %value, %gazonk ]
>   %value3.lcssa = phi i16 [ %value3, %foo ]
>   %value3.lcssa4 = phi i16 [ undef, %unreachable1 ], [ %value3, %bar ]
>   %value.lcssa.lcssa = phi i16 [ %value3.lcssa4, %exit2.pred ], [ %value.lcssa, %qqq ]
>
>
> My idea was that if we iterated in reverse order we would remove the use of `%value3.lcssa4` before trying to remove the PHI node defining that value.
>  But as seen above we wouldn't be able to remove the PHI node defining `%value3.lcssa` instead (as it is used by the first PHI node in the list).
>  So it will not help by changing the iteration order in case we desire to remove all those four PHI nodes.


No, you'd need a reverse postorder of the list of phistoremove, neither forward or backward order alone will help.  Reverse postorder will give you a reversed topo sort.

Insertion order you gave:

  %value.lcssa = phi i16 [ %value.lcssa, %www ], [ %value, %gazank ]
  %value.lcssa1 = phi i16 [ %value, %gazink ]
  %value.lcssa2 = phi i16 [ %value3.lcssa, %exit1.pred ], [ %value, %gazonk ]
  %value3.lcssa = phi i16 [ %value3, %foo ]
  %value3.lcssa4 = phi i16 [ undef, %unreachable1 ], [ %value3, %bar ]
  %value.lcssa.lcssa = phi i16 [ %value3.lcssa4, %exit2.pred ], [ %value.lcssa, %qqq ]
  %value.lcssa.lcssa5 = phi i16 [ %value.lcssa, %www ]

reverse postorder formed by following the operand tree of the insertion order you gave: would be (regular postorder numbers next to nodes)

  %value.lcssa.lcssa5 = phi i16 [ %value.lcssa, %www ] 7
  %value.lcssa.lcssa = phi i16 [ %value3.lcssa4, %exit2.pred ], [ %value.lcssa, %qqq ] 6
  %value3.lcssa4 = phi i16 [ undef, %unreachable1 ], [ %value3, %bar ] 5
  %value.lcssa2 = phi i16 [ %value3.lcssa, %exit1.pred ], [ %value, %gazonk ] 4
  %value3.lcssa = phi i16 [ %value3, %foo ] 3
  %value.lcssa1 = phi i16 [ %value, %gazink ] 2
  %value.lcssa = phi i16 [ %value.lcssa, %www ], [ %value, %gazank ] 1

In a single iteration, you should remove %value.lcssa.lcssa, then remove value3.lcssa4, then remove value.lcssa2,  then remove value3.lcssa

You can see even if you started with the above list order and redid postorder, it will still iterate in the same relatively right order.

(You have cycles here, which complicates guarantees)

  %value.lcssa1 = phi i16 [ %value, %gazink ] 7
  %value.lcssa2 = phi i16 [ %value3.lcssa, %exit1.pred ], [ %value, %gazonk ] 6
  %value3.lcssa = phi i16 [ %value3, %foo ] 5 
  %value.lcssa.lcssa = phi i16 [ %value3.lcssa4, %exit2.pred ], [ %value.lcssa, %qqq ] 4 
  %value3.lcssa4 = phi i16 [ undef, %unreachable1 ], [ %value3, %bar ] 3
  %value.lcssa.lcssa5 = phi i16 [ %value.lcssa, %www ] 2
  %value.lcssa = phi i16 [ %value.lcssa, %www ], [ %value, %gazank ] 1

remove value.lcssa2, remove value3.lcssa, remove value.lcssa.lccsa, remove value3.lcssa4


Repository:
  rL LLVM

https://reviews.llvm.org/D46422