NewGVN: Greatly enhance the ability of the NewGVN verifier to detect

issues, subsuming previous verifier.

llvm-svn: 298188

llvm/include/llvm/Transforms/Scalar/GVNExpression.h

@@ -65,7 +65,7 @@ public:
 
   static unsigned getEmptyKey() { return ~0U; }
   static unsigned getTombstoneKey() { return ~1U; }
-
+  bool operator!=(const Expression &Other) const { return !(*this == Other); }
   bool operator==(const Expression &Other) const {
     if (getOpcode() != Other.getOpcode())
       return false;

llvm/lib/Transforms/Scalar/NewGVN.cpp

@@ -176,6 +176,30 @@ struct CongruenceClass {
       : ID(ID), RepLeader(Leader), DefiningExpr(E) {}
 };
 
+// Return true if two congruence classes are equivalent to each other.  This
+// means
+// that every field but the ID number and the dead field are equivalent.
+bool areClassesEquivalent(const CongruenceClass *A, const CongruenceClass *B) {
+  if (A == B)
+    return true;
+  if ((A && !B) || (B && !A))
+    return false;
+
+  if (std::tie(A->StoreCount, A->RepLeader, A->RepStoredValue,
+               A->RepMemoryAccess) != std::tie(B->StoreCount, B->RepLeader,
+                                               B->RepStoredValue,
+                                               B->RepMemoryAccess))
+    return false;
+  if (A->DefiningExpr != B->DefiningExpr)
+    if (!A->DefiningExpr || !B->DefiningExpr ||
+        *A->DefiningExpr != *B->DefiningExpr)
+      return false;
+  // We need some ordered set
+  std::set<Value *> AMembers(A->Members.begin(), A->Members.end());
+  std::set<Value *> BMembers(B->Members.begin(), B->Members.end());
+  return AMembers == BMembers;
+}
+
 namespace llvm {
 template <> struct DenseMapInfo<const Expression *> {
   static const Expression *getEmptyKey() {
@@ -353,7 +377,6 @@ private:
   bool setMemoryAccessEquivTo(MemoryAccess *From, CongruenceClass *To);
   MemoryAccess *lookupMemoryAccessEquiv(MemoryAccess *) const;
   bool isMemoryAccessTop(const MemoryAccess *) const;
-
   // Ranking
   unsigned int getRank(const Value *) const;
   bool shouldSwapOperands(const Value *, const Value *) const;
@@ -387,13 +410,20 @@ private:
   void markLeaderChangeTouched(CongruenceClass *CC);
   void addPredicateUsers(const PredicateBase *, Instruction *);
 
+  // Main loop of value numbering
+  void iterateTouchedInstructions();
+
   // Utilities.
   void cleanupTables();
   std::pair<unsigned, unsigned> assignDFSNumbers(BasicBlock *, unsigned);
   void updateProcessedCount(Value *V);
   void verifyMemoryCongruency() const;
-  void verifyComparisons(Function &F);
+  void verifyIterationSettled(Function &F);
   bool singleReachablePHIPath(const MemoryAccess *, const MemoryAccess *) const;
+  BasicBlock *getBlockForValue(Value *V) const;
+  // Debug counter info.  When verifying, we have to reset the value numbering
+  // debug counter to the same state it started in to get the same results.
+  std::pair<int, int> StartingVNCounter;
 };
 } // end anonymous namespace
 
@@ -432,6 +462,16 @@ static std::string getBlockName(const BasicBlock *B) {
 }
 #endif
 
+// Get the basic block from an instruction/memory value.
+BasicBlock *NewGVN::getBlockForValue(Value *V) const {
+  if (auto *I = dyn_cast<Instruction>(V))
+    return I->getParent();
+  else if (auto *MP = dyn_cast<MemoryPhi>(V))
+    return MP->getBlock();
+  llvm_unreachable("Should have been able to figure out a block for our value");
+  return nullptr;
+}
+
 PHIExpression *NewGVN::createPHIExpression(Instruction *I) {
   BasicBlock *PHIBlock = I->getParent();
   auto *PN = cast<PHINode>(I);
@@ -1901,29 +1941,121 @@ void NewGVN::verifyMemoryCongruency() const {
   }
 }
 
-// Re-evaluate all the comparisons after value numbering and ensure they don't
+// Verify that the sparse propagation we did actually found the maximal fixpoint
-// change. If they changed, we didn't mark them touched properly.
+// We do this by storing the value to class mapping, touching all instructions,
-void NewGVN::verifyComparisons(Function &F) {
+// and redoing the iteration to see if anything changed.
+void NewGVN::verifyIterationSettled(Function &F) {
 #ifndef NDEBUG
-  for (auto &BB : F) {
+  if (DebugCounter::isCounterSet(VNCounter))
-    if (!ReachableBlocks.count(&BB))
+    DebugCounter::setCounterValue(VNCounter, StartingVNCounter);
-      continue;
+
-    for (auto &I : BB) {
+  // Note that we have to store the actual classes, as we may change existing
-      if (InstrDFS.lookup(&I) == 0)
+  // classes during iteration.  This is because our memory iteration propagation
+  // is not perfect, and so may waste a little work.  But it should generate
+  // exactly the same congruence classes we have now, with different IDs.
+  std::map<const Value *, CongruenceClass> BeforeIteration;
+
+  for (auto &KV : ValueToClass) {
+    if (auto *I = dyn_cast<Instruction>(KV.first))
+      // Skip unused/dead instructions.
+      if (InstrDFS.lookup(I) == 0)
         continue;
-      if (isa<CmpInst>(&I)) {
+    BeforeIteration.insert({KV.first, *KV.second});
-        auto *CurrentVal = ValueToClass.lookup(&I);
+  }
-        valueNumberInstruction(&I);
+
-        assert(CurrentVal == ValueToClass.lookup(&I) &&
+  TouchedInstructions.set();
-               "Re-evaluating comparison changed value");
+  TouchedInstructions.reset(0);
-      }
+  iterateTouchedInstructions();
+  DenseSet<std::pair<const CongruenceClass *, const CongruenceClass *>>
+      EqualClasses;
+  for (const auto &KV : ValueToClass) {
+    if (auto *I = dyn_cast<Instruction>(KV.first))
+      // Skip unused/dead instructions.
+      if (InstrDFS.lookup(I) == 0)
+        continue;
+    // We could sink these uses, but i think this adds a bit of clarity here as
+    // to what we are comparing.
+    auto *BeforeCC = &BeforeIteration.find(KV.first)->second;
+    auto *AfterCC = KV.second;
+    // Note that the classes can't change at this point, so we memoize the set
+    // that are equal.
+    if (!EqualClasses.count({BeforeCC, AfterCC})) {
+      assert(areClassesEquivalent(BeforeCC, AfterCC) &&
+             "Value number changed after main loop completed!");
+      EqualClasses.insert({BeforeCC, AfterCC});
     }
   }
 #endif
 }
 
+// This is the main value numbering loop, it iterates over the initial touched
+// instruction set, propagating value numbers, marking things touched, etc,
+// until the set of touched instructions is completely empty.
+void NewGVN::iterateTouchedInstructions() {
+  unsigned int Iterations = 0;
+  // Figure out where touchedinstructions starts
+  int FirstInstr = TouchedInstructions.find_first();
+  // Nothing set, nothing to iterate, just return.
+  if (FirstInstr == -1)
+    return;
+  BasicBlock *LastBlock = getBlockForValue(DFSToInstr[FirstInstr]);
+  while (TouchedInstructions.any()) {
+    ++Iterations;
+    // Walk through all the instructions in all the blocks in RPO.
+    // TODO: As we hit a new block, we should push and pop equalities into a
+    // table lookupOperandLeader can use, to catch things PredicateInfo
+    // might miss, like edge-only equivalences.
+    for (int InstrNum = TouchedInstructions.find_first(); InstrNum != -1;
+         InstrNum = TouchedInstructions.find_next(InstrNum)) {
+
+      // This instruction was found to be dead. We don't bother looking
+      // at it again.
+      if (InstrNum == 0) {
+        TouchedInstructions.reset(InstrNum);
+        continue;
+      }
+
+      Value *V = DFSToInstr[InstrNum];
+      BasicBlock *CurrBlock = getBlockForValue(V);
+
+      // If we hit a new block, do reachability processing.
+      if (CurrBlock != LastBlock) {
+        LastBlock = CurrBlock;
+        bool BlockReachable = ReachableBlocks.count(CurrBlock);
+        const auto &CurrInstRange = BlockInstRange.lookup(CurrBlock);
+
+        // If it's not reachable, erase any touched instructions and move on.
+        if (!BlockReachable) {
+          TouchedInstructions.reset(CurrInstRange.first, CurrInstRange.second);
+          DEBUG(dbgs() << "Skipping instructions in block "
+                       << getBlockName(CurrBlock)
+                       << " because it is unreachable\n");
+          continue;
+        }
+        updateProcessedCount(CurrBlock);
+      }
+
+      if (auto *MP = dyn_cast<MemoryPhi>(V)) {
+        DEBUG(dbgs() << "Processing MemoryPhi " << *MP << "\n");
+        valueNumberMemoryPhi(MP);
+      } else if (auto *I = dyn_cast<Instruction>(V)) {
+        valueNumberInstruction(I);
+      } else {
+        llvm_unreachable("Should have been a MemoryPhi or Instruction");
+      }
+      updateProcessedCount(V);
+      // Reset after processing (because we may mark ourselves as touched when
+      // we propagate equalities).
+      TouchedInstructions.reset(InstrNum);
+    }
+  }
+  NumGVNMaxIterations = std::max(NumGVNMaxIterations.getValue(), Iterations);
+}
+
 // This is the main transformation entry point.
 bool NewGVN::runGVN() {
+  if (DebugCounter::isCounterSet(VNCounter))
+    StartingVNCounter = DebugCounter::getCounterValue(VNCounter);
   bool Changed = false;
   NumFuncArgs = F.arg_size();
   MSSAWalker = MSSA->getWalker();
@@ -1992,71 +2124,10 @@ bool NewGVN::runGVN() {
   ReachableBlocks.insert(&F.getEntryBlock());
 
   initializeCongruenceClasses(F);
-
+  iterateTouchedInstructions();
-  unsigned int Iterations = 0;
-  // We start out in the entry block.
-  BasicBlock *LastBlock = &F.getEntryBlock();
-  while (TouchedInstructions.any()) {
-    ++Iterations;
-    // Walk through all the instructions in all the blocks in RPO.
-    // TODO: As we hit a new block, we should push and pop equalities into a
-    // table lookupOperandLeader can use, to catch things PredicateInfo
-    // might miss, like edge-only equivalences.
-    for (int InstrNum = TouchedInstructions.find_first(); InstrNum != -1;
-         InstrNum = TouchedInstructions.find_next(InstrNum)) {
-
-      // This instruction was found to be dead. We don't bother looking
-      // at it again.
-      if (InstrNum == 0) {
-        TouchedInstructions.reset(InstrNum);
-        continue;
-      }
-
-      Value *V = DFSToInstr[InstrNum];
-      BasicBlock *CurrBlock = nullptr;
-
-      if (auto *I = dyn_cast<Instruction>(V))
-        CurrBlock = I->getParent();
-      else if (auto *MP = dyn_cast<MemoryPhi>(V))
-        CurrBlock = MP->getBlock();
-      else
-        llvm_unreachable("DFSToInstr gave us an unknown type of instruction");
-
-      // If we hit a new block, do reachability processing.
-      if (CurrBlock != LastBlock) {
-        LastBlock = CurrBlock;
-        bool BlockReachable = ReachableBlocks.count(CurrBlock);
-        const auto &CurrInstRange = BlockInstRange.lookup(CurrBlock);
-
-        // If it's not reachable, erase any touched instructions and move on.
-        if (!BlockReachable) {
-          TouchedInstructions.reset(CurrInstRange.first, CurrInstRange.second);
-          DEBUG(dbgs() << "Skipping instructions in block "
-                       << getBlockName(CurrBlock)
-                       << " because it is unreachable\n");
-          continue;
-        }
-        updateProcessedCount(CurrBlock);
-      }
-
-      if (auto *MP = dyn_cast<MemoryPhi>(V)) {
-        DEBUG(dbgs() << "Processing MemoryPhi " << *MP << "\n");
-        valueNumberMemoryPhi(MP);
-      } else if (auto *I = dyn_cast<Instruction>(V)) {
-        valueNumberInstruction(I);
-      } else {
-        llvm_unreachable("Should have been a MemoryPhi or Instruction");
-      }
-      updateProcessedCount(V);
-      // Reset after processing (because we may mark ourselves as touched when
-      // we propagate equalities).
-      TouchedInstructions.reset(InstrNum);
-    }
-  }
-  NumGVNMaxIterations = std::max(NumGVNMaxIterations.getValue(), Iterations);
 #ifndef NDEBUG
   verifyMemoryCongruency();
-  verifyComparisons(F);
+  verifyIterationSettled(F);
 #endif
 
   Changed |= eliminateInstructions(F);
@@ -2093,13 +2164,6 @@ static bool alwaysAvailable(Value *V) {
   return isa<Constant>(V) || isa<Argument>(V);
 }
 
-// Get the basic block from an instruction/value.
-static BasicBlock *getBlockForValue(Value *V) {
-  if (auto *I = dyn_cast<Instruction>(V))
-    return I->getParent();
-  return nullptr;
-}
-
 struct NewGVN::ValueDFS {
   int DFSIn = 0;
   int DFSOut = 0;