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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a subscriber: aeubanks.
nickdesaulniers 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));
----------------
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,
    ~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2699
+  if (Changed) {
+    if (MD)
+      MD->invalidateCachedPredecessors();
----------------
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.


================
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
+
----------------
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/
?


================
Comment at: llvm/test/Transforms/GVN/critical-edge-split-failure.ll:13
+
+define dso_local i32 @arch_static_branch() {
+entry:
----------------
fhahn wrote:
> is this function needed?
This is what was produced by running both `llvm-reduce` and `bugpoint`; I wonder if they don't remove functions referred to by block addresses? (Cycle counter/cycle breaker problem).  Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94996