[PATCH] D132704: [LCSSA] Re-use suitable PHI if available instead creating new ones.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LCSSA.cpp:176
+        }
+      }
       Builder.SetInsertPoint(&ExitBB->front());
----------------
fhahn wrote:
> efriedma wrote:
> > fhahn wrote:
> > > efriedma wrote:
> > > > Is the new code to prevent creating identical PHI nodes part of the fix, or just an optimization?  It isn't obviously related.
> > > Preventing identical phis is the main part of the fix.  The issue is that currently scev expansion can create different LCSSA phis when expanding the same expression (since we changed scev to look through trivial phis)
> > > 
> > > If those are then used as incoming values for phis with multiple matching predecessors we create invalid ir .
> > I'm sorry, I really don't follow how having multiple PHI nodes that use the value messes up the algorithm.  (If someone else does, feel free to take over reviewing...)
> Sorry for the confusion. Having multiple multiple phi nodes for the same values isn't a problem for the LCSSA construction algorithm itself. For the LCSSA construction algorithm the change is purely an optimization.
> 
> The bug fix is that is also addresses a problem for  a user of the construction algorithm (SCEVExpander). Creating duplicated phis when expanding the same expression multiple times can cause issues with uses of the expanded expressions. 
> 
> In the added test case `pr57000`, `IndVars` updates the incoming values of an LCSSA phi node with multiple incoming edges from the same block. It expands the same SCEV expression once for each edge and expanding it requires creating new LCSSA phi nodes (using the construction algorithm,). With the patch, LCSSA expansion will return a single PHI, whereas before it would create duplicated phis for each expansion, resulting in a phi node with different incoming values for edges from the same block.
> 
> I hope this helps to clarify things a bit.
Dug a bit.  I think I understand: rewriteLoopExitValues calls expandCodeFor in a loop.  The way that loop is written, it implicitly assumes that if it calls expandCodeFor multiple times with the same operands, it will receive the same result.  (Otherwise, it ends up setting the operands of the PHI it's rewriting to invalid values.)

There are other ways you could fix that particular issue.  For example, we could just avoid repeated calls to expandCodeFor in rewriteLoopExitValues, which would be both faster, and more obviously correct.

----

I'm a little concerned that the current patch could end up iterating over ExitBB->phis() repeatedly, which would be effectively O(N^2).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132704