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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LCSSA.cpp:176
+        }
+      }
       Builder.SetInsertPoint(&ExitBB->front());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132704