fixup~ Revert changes that precompute for getHashValue.

Author: michaelmaitland

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

@@ -7442,16 +7442,12 @@ static bool simplifySwitchOfCmpIntrinsic(SwitchInst *SI, IRBuilderBase &Builder,
 /// PHINode::getIncomingValueForBlock is O(|Preds|), so we'd like to avoid
 /// calling this function on each BasicBlock every time isEqual is called,
 /// especially since the same BasicBlock may be passed as an argument multiple
-/// times. To do this, we can precompute a map, PhiPredIVs, of PHINode -> Pred
-/// BasicBlock -> IncomingValue and add it in the Wrapper so isEqual can do O(1)
-/// checking of the incoming values. We also want to use the incoming Phi
-/// values of a getCaseSuccessor to calculate the hash value. In order to avoid
-/// iterating all of the successor Phis on evry call to getHashValue, we
-/// precompute a list of incoming values from getCaseSucessor, PhiVals.
+/// times. To do this, we can precompute a map of PHINode -> Pred BasicBlock ->
+/// IncomingValue and add it in the Wrapper so isEqual can do O(1) checking
+/// of the incoming values.
 struct CaseHandleWrapper {
   const SwitchInst::CaseHandle Case;
   DenseMap<PHINode *, DenseMap<BasicBlock *, Value *>> *PhiPredIVs;
-  DenseMap<BasicBlock *, SmallVector<Value *>> *PhiVals;
 };
 
 namespace llvm {
@@ -7473,14 +7469,22 @@ template <> struct DenseMapInfo<const CaseHandleWrapper *> {
            "Expected unconditional branches to have one successor");
     assert(Succ->size() == 1 && "Expected just a single branch in the BB");
 
-    // Since the BB is just a single BranchInst with a single successor, we hash
-    // the BB and the incoming Values of its successor incoming PHI values.
-    // Initially, we tried to just use the successor BB as the hash, but this
-    // has better performance.
+    // Since we assume the BB is just a single BranchInst with a single
+    // succsessor, we hash as the BB and the incoming Values of its successor
+    // PHIs. Initially, we tried to just use the successor BB as the hash, but
+    // this had poor performance. We find that the extra computation of getting
+    // the incoming PHI values here leads to better performance on overall Set
+    // performance. We also tried to build a map from BB -> Succs.IncomingValues
+    // ahead of time and passing it in CaseHandleWrapper, but this slowed down
+    // the average compile time without having any impact on the worst case
+    // compile time.
     BasicBlock *BB = BI->getSuccessor(0);
-    auto &Vals = (*CHW->PhiVals)[BB];
-    return hash_combine(hash_value(BB),
-                        hash_combine_range(Vals.begin(), Vals.end()));
+    SmallVector<Value *> PhiValsForBB;
+    for (PHINode &Phi : BB->phis())
+      PhiValsForBB.emplace_back((*CHW->PhiPredIVs)[&Phi][BB]);
+
+    return hash_combine(
+        BB, hash_combine_range(PhiValsForBB.begin(), PhiValsForBB.end()));
   }
   static bool isEqual(const CaseHandleWrapper *LHS,
                       const CaseHandleWrapper *RHS) {
@@ -7530,7 +7534,6 @@ bool SimplifyCFGOpt::simplifyDuplicateSwitchArms(SwitchInst *SI) {
   SmallPtrSet<PHINode *, 8> Phis;
   SmallPtrSet<BasicBlock *, 8> Seen;
   DenseMap<PHINode *, DenseMap<BasicBlock *, Value *>> PhiPredIVs;
-  DenseMap<BasicBlock *, SmallVector<Value *>> PhiVals;
   std::vector<CaseHandleWrapper> Cases;
   Cases.reserve(SI->getNumCases());
   for (auto &Case : SI->cases()) {
@@ -7558,32 +7561,22 @@ bool SimplifyCFGOpt::simplifyDuplicateSwitchArms(SwitchInst *SI) {
       continue;
 
     if (Seen.insert(BB).second) {
-      // Keep track of which PHIs we need as keys in PhiPredIVs.
-      for (BasicBlock *Succ : BI->successors()) {
-        for (PHINode &Phi : Succ->phis()) {
+      // Keep track of which PHIs we need as keys in PhiPredIVs below.
+      for (BasicBlock *Succ : BI->successors())
+        for (PHINode &Phi : Succ->phis())
           Phis.insert(&Phi);
-          PhiVals[BB] = SmallVector<Value *>();
-        }
-      }
     }
-    Cases.emplace_back(CaseHandleWrapper{Case, &PhiPredIVs, &PhiVals});
+    Cases.emplace_back(CaseHandleWrapper{Case, &PhiPredIVs});
   }
 
-  // Precompute the data structures to improve performance of isEqual and
-  // getHashValue for CaseHandleWrapper.
+  // Precompute a data structure to improve performance of isEqual for
+  // CaseHandleWrapper.
   PhiPredIVs.reserve(Phis.size());
   for (PHINode *Phi : Phis) {
     PhiPredIVs[Phi] =
         DenseMap<BasicBlock *, Value *>(Phi->getNumIncomingValues());
-    for (Use &IV : Phi->incoming_values()) {
-      BasicBlock *BB = Phi->getIncomingBlock(IV);
-      PhiPredIVs[Phi].insert({BB, IV.get()});
-      // Only need to track PhiVals for BBs we've added as keys in the loop
-      // above.
-      auto Res = PhiVals.find(BB);
-      if (Res != PhiVals.end())
-        Res->second.emplace_back(IV.get());
-    }
+    for (auto &IV : Phi->incoming_values())
+      PhiPredIVs[Phi].insert({Phi->getIncomingBlock(IV), IV.get()});
   }
 
   // Build a set such that if the CaseHandleWrapper exists in the set and