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

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1393
+  auto *Term = Pred->getTerminator();
+  if (Term->getNumSuccessors() != 2 || Term->isExceptionalTerminator())
+    return nullptr;
----------------
mkazantsev wrote:
> Do you care about `switch` with 2 branches? If not, maybe then `match(m_Br(m_Value(), m_BasicBlock(IfTrue), m_BasicBlock(IfFalse)))`?
I prefer to include switch case, this is a benefit without any extra cost.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1396
+  auto *SuccBB = Term->getSuccessor(0);
+  if (SuccBB == LoadBB)
+    SuccBB = Term->getSuccessor(1);
----------------
mkazantsev wrote:
> Will there be a problem if `Term->getSuccessor(0) == Term->getSuccessor(1)`? Any tests for it?
If Term->getSuccessor(0) == Term->getSuccessor(1), then SuccBB will have two predecessors, the next statement should exit immediately. Added a test case for it.


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


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1583
+  // the movable load.
+  MapVector<BasicBlock *, LoadInst *> CriticalEdgePredAndLoad;
   for (BasicBlock *Pred : predecessors(LoadBB)) {
----------------
mkazantsev wrote:
> 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.
It looks similar to your comment in findLoadToHoistIntoPred. Either the switch BB contains an identical load, nothing should be handled. Or switch BB doesn't contains an identical load, findLoadToHoistIntoPred returns nullptr because of too many edges. Test added.


================
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
 
----------------
mkazantsev wrote:
> Why change that?
Because with this optimization, both cases generate same result. The PRE of load %desc now can be detected and moved to entry block.


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

https://reviews.llvm.org/D141712