[PATCH] D141712: [GVN] Improve PRE on load instructions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.

Some minor nits & style proposals; could you please add more tests for weird corner cases, specifically same block being a pred multiple times?



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1393
+  auto *Term = Pred->getTerminator();
+  if (Term->getNumSuccessors() != 2 || Term->isExceptionalTerminator())
+    return nullptr;
----------------
Do you care about `switch` with 2 branches? If not, maybe then `match(m_Br(m_Value(), m_BasicBlock(IfTrue), m_BasicBlock(IfFalse)))`?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1396
+  auto *SuccBB = Term->getSuccessor(0);
+  if (SuccBB == LoadBB)
+    SuccBB = Term->getSuccessor(1);
----------------
Will there be a problem if `Term->getSuccessor(0) == Term->getSuccessor(1)`? Any tests for it?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1398
+    SuccBB = Term->getSuccessor(1);
+  if (!SuccBB->getSinglePredecessor() || SuccBB->isEHPad())
+    return nullptr;
----------------
Do you really need to check `EHPad` after you've already checked for `isExceptionalTerminator`?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1401
+
+  int NumInsts = MaxNumInsnsPerBlock;
+  for (Instruction &Inst : *SuccBB) {
----------------
nit: unsigned to signed conversion, might be warning in compiler


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1402
+  int NumInsts = MaxNumInsnsPerBlock;
+  for (Instruction &Inst : *SuccBB) {
+    if (Inst.isIdenticalTo(Load)) {
----------------
Maybe
```
for (Instruction &Inst : *SuccBB) {
  if (!Inst.isIdenticalTo(Load))
    continue;
  // same code with reduced nest
}
```


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1410
+
+      return nullptr;
+    }
----------------
Add a comment that, if one identical load already depends on something, then there is no point to look further?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1483
+      if (I != CriticalEdgePredAndLoad->end()) {
+        LoadInst *OldLoad = I->second;
+        OldLoad->replaceAllUsesWith(NewLoad);
----------------
Add some statistic here?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1583
+  // the movable load.
+  MapVector<BasicBlock *, LoadInst *> CriticalEdgePredAndLoad;
   for (BasicBlock *Pred : predecessors(LoadBB)) {
----------------
What if the same block goes into LoadBB multiple times? Smth like
```
   switch cond
      case 1: LoadBB
      case 2: LoadBB
      case 3: LoadBB
      default: LoadBB

```
Will this work correctly for this case? Please add some tests for situations like this.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1623
 
-      CriticalEdgePred.push_back(Pred);
+      if (LoadInst *LI = findLoadToHoistIntoPred(Pred, LoadBB, Load)) {
+        CriticalEdgePredAndLoad[Pred] = LI;
----------------
nit: `{ }` not needed


================
Comment at: llvm/test/Transforms/GVN/PRE/2017-06-28-pre-load-dbgloc.ll:3
-; RUN: opt < %s -passes=gvn -S | FileCheck %s --check-prefixes=ALL,GVN
-; RUN: opt < %s -passes=gvn,instcombine -S | FileCheck %s --check-prefixes=ALL,INSTCOMBINE
 
----------------
Why change that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141712