[llvm] r275761 - [GVNHoist] Change the key for VNtoInsns to a pair

[David Majnemer via llvm-commits]

Author: majnemer
Date: Mon Jul 18 01:11:37 2016
New Revision: 275761

URL: http://llvm.org/viewvc/llvm-project?rev=275761&view=rev
Log:
[GVNHoist] Change the key for VNtoInsns to a pair

While debugging GVNHoist, I found it confusing that the entries in a
VNtoInsns were not always value numbers.  They _usually_ were except for
StoreInst in which case they were a hash of two different value numbers.

This leads to two observations:
- It is more difficult to debug things when the semantic contents of
  VNtoInsns changes over time.
- Using a single value number is not much cheaper, the value of
  VNtoInsns is a SmallVector.
- It is not immediately clear what the algorithm would do if there were
  hash collisions in the StoreInst case.

Using a DenseMap of std::pair sidesteps all of this.

N.B.  The changes in the test were due their sensitivity to the
iteration order of VNtoInsns which has changed.

Modified:
    llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
    llvm/trunk/test/Transforms/GVN/hoist.ll

Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=275761&r1=275760&r2=275761&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Mon Jul 18 01:11:37 2016
@@ -81,8 +81,12 @@ public:
   }
 };
 
-// A map from a VN (value number) to all the instructions with that VN.
-typedef DenseMap<unsigned, SmallVector<Instruction *, 4>> VNtoInsns;
+// A map from a pair of VNs to all the instructions with those VNs.
+typedef DenseMap<std::pair<unsigned, unsigned>, SmallVector<Instruction *, 4>>
+    VNtoInsns;
+// An invalid value number Used when inserting a single value number into
+// VNtoInsns.
+enum { InvalidVN = ~2U };
 
 // Records all scalar instructions candidate for code hoisting.
 class InsnInfo {
@@ -93,7 +97,7 @@ public:
   void insert(Instruction *I, GVN::ValueTable &VN) {
     // Scalar instruction.
     unsigned V = VN.lookupOrAdd(I);
-    VNtoScalars[V].push_back(I);
+    VNtoScalars[{V, InvalidVN}].push_back(I);
   }
 
   const VNtoInsns &getVNTable() const { return VNtoScalars; }
@@ -108,7 +112,7 @@ public:
   void insert(LoadInst *Load, GVN::ValueTable &VN) {
     if (Load->isSimple()) {
       unsigned V = VN.lookupOrAdd(Load->getPointerOperand());
-      VNtoLoads[V].push_back(Load);
+      VNtoLoads[{V, InvalidVN}].push_back(Load);
     }
   }
 
@@ -128,8 +132,7 @@ public:
     // Hash the store address and the stored value.
     Value *Ptr = Store->getPointerOperand();
     Value *Val = Store->getValueOperand();
-    VNtoStores[hash_combine(VN.lookupOrAdd(Ptr), VN.lookupOrAdd(Val))]
-        .push_back(Store);
+    VNtoStores[{VN.lookupOrAdd(Ptr), VN.lookupOrAdd(Val)}].push_back(Store);
   }
 
   const VNtoInsns &getVNTable() const { return VNtoStores; }
@@ -148,13 +151,14 @@ public:
     // onlyReadsMemory will be handled as a Load instruction,
     // all other calls will be handled as stores.
     unsigned V = VN.lookupOrAdd(Call);
+    auto Entry = std::make_pair(V, InvalidVN);
 
     if (Call->doesNotAccessMemory())
-      VNtoCallsScalars[V].push_back(Call);
+      VNtoCallsScalars[Entry].push_back(Call);
     else if (Call->onlyReadsMemory())
-      VNtoCallsLoads[V].push_back(Call);
+      VNtoCallsLoads[Entry].push_back(Call);
     else
-      VNtoCallsStores[V].push_back(Call);
+      VNtoCallsStores[Entry].push_back(Call);
   }
 
   const VNtoInsns &getScalarVNTable() const { return VNtoCallsScalars; }

Modified: llvm/trunk/test/Transforms/GVN/hoist.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/hoist.ll?rev=275761&r1=275760&r2=275761&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/GVN/hoist.ll (original)
+++ llvm/trunk/test/Transforms/GVN/hoist.ll Mon Jul 18 01:11:37 2016
@@ -8,9 +8,9 @@ target triple = "x86_64-unknown-linux-gn
 ;
 ; CHECK-LABEL: @scalarsHoisting
 ; CHECK: fsub
-; CHECK: fmul
 ; CHECK: fsub
 ; CHECK: fmul
+; CHECK: fmul
 ; CHECK-NOT: fmul
 ; CHECK-NOT: fsub
 define float @scalarsHoisting(float %d, float %min, float %max, float %a) {
@@ -48,9 +48,9 @@ if.end:
 ; CHECK: load
 ; CHECK: load
 ; CHECK: fsub
-; CHECK: fmul
 ; CHECK: fsub
 ; CHECK: fmul
+; CHECK: fmul
 ; CHECK-NOT: load
 ; CHECK-NOT: fmul
 ; CHECK-NOT: fsub
@@ -223,10 +223,10 @@ if.end:
 
 ; Check that all independent expressions are hoisted.
 ; CHECK-LABEL: @independentScalarsHoisting
-; CHECK: fmul
 ; CHECK: fadd
-; CHECK: fdiv
 ; CHECK: fsub
+; CHECK: fdiv
+; CHECK: fmul
 ; CHECK-NOT: fsub
 ; CHECK-NOT: fdiv
 ; CHECK-NOT: fmul