[PATCH] D94996: [GVN] do not repeat PRE on failure to split critical edge

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2695
     std::pair<Instruction *, unsigned> Edge = toSplit.pop_back_val();
-    SplitCriticalEdge(Edge.first, Edge.second,
-                      CriticalEdgeSplittingOptions(DT, LI, MSSAU));
+    Changed |= !!SplitCriticalEdge(Edge.first, Edge.second,
+                                   CriticalEdgeSplittingOptions(DT, LI, MSSAU));
----------------
nickdesaulniers wrote:
> fhahn wrote:
> > do we need `!!` here?
> unless you'd prefer a comparison `!= nullptr`? Otherwise:
> ```
> ../lib/Transforms/Scalar/GVN.cpp:2695:13: error: invalid operands to binary expression ('bool' and 'llvm::BasicBlock *')
>     Changed |= SplitCriticalEdge(Edge.first, Edge.second,
>     ~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
oh right, `!= nullptr` seems clearer to me.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2699
+  if (Changed) {
+    if (MD)
+      MD->invalidateCachedPredecessors();
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > fhahn wrote:
> > > This code seems redundant, right, because it is already done by `splitCriticalEdges`?
> > Different code paths; the method `GVN::splitCriticalEdges` that accepts arguments is called from different places at different points during GVN.
> > 
> > That method is used to splitting individual edges eagerly, then invalidating cached predecessors.
> > 
> > This method is for splitting a list of edges that were deferred, then invalidating cached predecessors.
> The other method probably shouldn't be plural. s/splitCriticalEdges/splitCriticalEdge/
ah right, this doesn't actually call `GVN::splitCriticalEdges`. Nevermind then.


================
Comment at: llvm/test/Transforms/GVN/critical-edge-split-failure.ll:2
+; RUN: opt -gvn -S -o - %s | FileCheck %s
+; RUN: opt -passes=gvn -S -o - %s | FileCheck %s
+
----------------
aeubanks wrote:
> nickdesaulniers wrote:
> > This is yet another case where NPM test passes before/after a patch that would be red with LPM. cc @aeubanks 
> > 
> > I don't know if this test should be under
> > 
> > llvm/test/Transforms/GVN/PRE/
> > rather than
> > llvm/test/Transforms/GVN/
> > ?
> This one is much more surprising, I can't think of anything off the top of my head that would cause the output of running GVN to change between the two pass managers.
missing `aa-pipeline` option?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94996