Merge pull request #27028 from nate-chandler/update-instruction-worklist

swift-ci · web-flow · commit 902da317e295 · 2019-09-06T11:10:00.000-07:00
diff --git a/include/swift/Basic/BlotSetVector.h b/include/swift/Basic/BlotSetVector.h
@@ -47,111 +47,132 @@ namespace swift {
 template <typename ValueT, typename VectorT = std::vector<Optional<ValueT>>,
           typename MapT = llvm::DenseMap<ValueT, unsigned>>
 class BlotSetVector {
-  VectorT Vector;
-  MapT Map;
+  VectorT vector;
+  MapT map;
 
 public:
   /// Construct an empty BlotSetVector.
   BlotSetVector() {}
 
-  bool empty() const { return Vector.empty(); }
+  bool empty() const { return vector.empty(); }
 
-  unsigned size() const { return Vector.size(); }
+  unsigned size() const { return vector.size(); }
 
   using iterator = typename VectorT::iterator;
   using const_iterator = typename VectorT::const_iterator;
-  iterator begin() { return Vector.begin(); }
-  iterator end() { return Vector.end(); }
-  const_iterator begin() const { return Vector.begin(); }
-  const_iterator end() const { return Vector.end(); }
+  iterator begin() { return vector.begin(); }
+  iterator end() { return vector.end(); }
+  const_iterator begin() const { return vector.begin(); }
+  const_iterator end() const { return vector.end(); }
   llvm::iterator_range<const_iterator> getRange() const {
     return {begin(), end()};
   }
 
   using const_reverse_iterator = typename VectorT::const_reverse_iterator;
-  const_reverse_iterator rbegin() const { return Vector.rbegin(); }
-  const_reverse_iterator rend() const { return Vector.rend(); }
+  const_reverse_iterator rbegin() const { return vector.rbegin(); }
+  const_reverse_iterator rend() const { return vector.rend(); }
   llvm::iterator_range<const_reverse_iterator> getReverseRange() const {
     return {rbegin(), rend()};
   }
 
   const Optional<ValueT> &operator[](unsigned n) const {
-    assert(n < Vector.size() && "Out of range!");
-    return Vector[n];
+    assert(n < vector.size() && "Out of range!");
+    return vector[n];
   }
 
-  /// Insert \p V into the SetVector if it is not in the array and return the
-  /// index of \p V in the Set Vector. If \p V is already in the SetVector, just
-  /// return its index in the array.
-  unsigned insert(const ValueT &V) {
-    auto Iter = Map.find(V);
-    if (Iter != Map.end())
-      return Iter->second;
-
-    unsigned Index = Vector.size();
-    Map[V] = Index;
-    Vector.push_back(V);
-    return Index;
+  /// Grow the set vector so that it can contain at least \p capacity items
+  /// before resizing again.
+  ///
+  /// \param capacity The minimum size that the set vector will be able to grow
+  ///                 to before a resize is required to insert additional items.
+  void reserve(unsigned capacity) {
+    vector.reserve(capacity);
+    map.reserve(capacity);
   }
 
-  /// Replace \p V1 with \p V2 placing \p V2 into the position in the array
-  /// where V1 used to be. If \p V2 is already in the set, this just erases \p
-  /// V1.
-  void replace(const ValueT &V1, const ValueT &V2) {
-    auto Iter1 = Map.find(V1);
-    assert(Iter1 != Map.end() && "Cannot replace value that is not in set");
-    unsigned V1Index = Iter1->second;
-    Map.erase(V1);
-
-    auto Iter2 = Map.find(V2);
-    if (Iter2 != Map.end()) {
-      Vector[V1Index] = None;
+  /// Insert \p value into the SetVector if it is not there already.
+  ///
+  /// \param value The item to insert if not already present.
+  ///
+  /// \returns Both the index of the item and whether it was inserted in a pair.
+  ///          If the item was already present, its preexisting index along with
+  ///          false will be returned.  If the item was newly added, its new
+  ///          index along with true will be returned.
+  std::pair<unsigned, bool> insert(const ValueT &value) {
+    auto iterator = map.find(value);
+    if (iterator != map.end())
+      return {iterator->second, false};
+
+    unsigned index = vector.size();
+    map[value] = index;
+    vector.push_back(value);
+    return {index, true};
+  }
+
+  /// Replace \p value1 with \p value2 placing \p value2 into the position in
+  /// the array where value1 used to be. If \p value2 is already in the set,
+  /// this just erases \p value1.
+  void replace(const ValueT &value1, const ValueT &value2) {
+    auto iterator1 = map.find(value1);
+    assert(iterator1 != map.end() && "Cannot replace value that is not in set");
+    unsigned index1 = iterator1->second;
+    map.erase(value1);
+
+    auto iterator2 = map.find(value2);
+    if (iterator2 != map.end()) {
+      vector[index1] = None;
       return;
     }
 
-    Map[V2] = V1Index;
-    Vector[V1Index] = V2;
+    map[value2] = index1;
+    vector[index1] = value2;
   }
 
-  /// Erase the value \p V if it is in the set. Returns true if V was
+  /// Erase the value \p value if it is in the set. Returns true if value was
   /// successfully erased and false otherwise.
-  bool erase(const ValueT &V) {
-    auto Iter = Map.find(V);
-    if (Iter == Map.end())
+  bool erase(const ValueT &value) {
+    auto iterator = map.find(value);
+    if (iterator == map.end())
       return false;
-    unsigned Index = Iter->second;
-    Map.erase(Iter);
-    Vector[Index] = None;
+    unsigned index = iterator->second;
+    map.erase(iterator);
+    vector[index] = None;
     return true;
   }
 
   /// Return the last element of the blot map vector. Will be None if blotted.
   Optional<ValueT> pop_back_val() {
-    auto result = Vector.pop_back_val();
+    auto result = vector.pop_back_val();
     if (!result)
       return result;
-    Map.erase(*result);
+    map.erase(*result);
     return result;
   }
 
-  /// Attempt to lookup the index of \p V. Returns None upon failure and the
+  /// Attempt to lookup the index of \p value. Returns None upon failure and the
   /// value on success.
-  Optional<unsigned> getIndex(const ValueT &V) {
-    auto Iter = Map.find(V);
-    if (Iter == Map.end())
+  Optional<unsigned> getIndex(const ValueT &value) {
+    auto iterator = map.find(value);
+    if (iterator == map.end())
       return None;
-    return Iter->second;
+    return iterator->second;
+  }
+
+  /// Clear the backing vector and map.
+  void clear() {
+    vector.clear();
+    map.clear();
   }
 };
 
-template <typename ValueT, unsigned N,
-          typename VectorT = llvm::SmallVector<Optional<ValueT>, N>,
-          typename MapT = llvm::SmallDenseMap<ValueT, unsigned, N>>
-class SmallBlotSetVector : public BlotSetVector<ValueT, VectorT, MapT> {
+template <typename ValueT, unsigned N>
+class SmallBlotSetVector
+    : public BlotSetVector<ValueT, llvm::SmallVector<Optional<ValueT>, N>,
+                           llvm::SmallDenseMap<ValueT, unsigned, N>> {
 public:
   SmallBlotSetVector() {}
 };
 
-} // end swift namespace
+} // namespace swift
 
 #endif // SWIFT_BASIC_BLOTSETVECTOR_H
diff --git a/include/swift/SIL/SILInstructionWorklist.h b/include/swift/SIL/SILInstructionWorklist.h
@@ -24,92 +24,128 @@
 ///
 //===----------------------------------------------------------------------===//
 
+#include "swift/Basic/BlotSetVector.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SIL/SILValue.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 
 namespace swift {
 
+class SILInstructionWorklistBase {
+  const char *loggingName;
+
+protected:
+  SILInstructionWorklistBase(const char *loggingName)
+      : loggingName(loggingName){};
+  void withDebugStream(
+      std::function<void(llvm::raw_ostream &stream, const char *loggingName)>
+          perform);
+};
+
 /// Manages a list of instructions awaiting visitation.
-class SILInstructionWorklist final {
-  llvm::SmallVector<SILInstruction *, 256> worklist;
-  llvm::DenseMap<SILInstruction *, unsigned> worklistMap;
-  StringRef loggingName;
+template <typename VectorT = std::vector<SILInstruction *>,
+          typename MapT = llvm::DenseMap<SILInstruction *, unsigned>>
+class SILInstructionWorklist : SILInstructionWorklistBase {
+  BlotSetVector<SILInstruction *, VectorT, MapT> worklist;
 
   void operator=(const SILInstructionWorklist &rhs) = delete;
   SILInstructionWorklist(const SILInstructionWorklist &worklist) = delete;
 
 public:
   SILInstructionWorklist(const char *loggingName = "InstructionWorklist")
-      : loggingName(loggingName) {}
+      : SILInstructionWorklistBase(loggingName) {}
 
   /// Returns true if the worklist is empty.
   bool isEmpty() const { return worklist.empty(); }
 
   /// Add the specified instruction to the worklist if it isn't already in it.
-  void add(SILInstruction *instruction);
+  void add(SILInstruction *instruction) {
+    if (worklist.insert(instruction).second) {
+      withDebugStream([&](llvm::raw_ostream &stream, StringRef loggingName) {
+        stream << loggingName << ": ADD: " << *instruction << '\n';
+      });
+    }
+  }
 
-  /// If the given ValueBase is a SILInstruction add it to the worklist.
-  void addValue(ValueBase *value) {
-    if (auto *instruction = value->getDefiningInstruction())
+  /// If the given SILValue is a SILInstruction add it to the worklist.
+  void addValue(SILValue value) {
+    if (auto *instruction = value->getDefiningInstruction()) {
       add(instruction);
+    }
   }
 
   /// Add the given list of instructions in reverse order to the worklist. This
   /// routine assumes that the worklist is empty and the given list has no
   /// duplicates.
-  void addInitialGroup(ArrayRef<SILInstruction *> list);
+  void addInitialGroup(ArrayRef<SILInstruction *> list) {
+    assert(worklist.empty() && "worklist must be empty to add initial group");
+    worklist.reserve(list.size() + 16);
+    withDebugStream([&](llvm::raw_ostream &stream, StringRef loggingName) {
+      stream << loggingName << ": ADDING: " << list.size()
+             << " instrs to worklist\n";
+    });
+    while (!list.empty()) {
+      SILInstruction *instruction = list.back();
+      list = list.drop_back();
+      worklist.insert(instruction);
+    }
+  }
 
   // If instruction is in the worklist, remove it.
-  void remove(SILInstruction *instruction) {
-    auto iterator = worklistMap.find(instruction);
-    if (iterator == worklistMap.end())
-      return; // Not in worklist.
-
-    // Don't bother moving everything down, just null out the slot. We will
-    // check before we process any instruction if it is null.
-    worklist[iterator->second] = nullptr;
-    worklistMap.erase(iterator);
-  }
+  void erase(SILInstruction *instruction) { worklist.erase(instruction); }
 
   /// Remove the top element from the worklist.
-  SILInstruction *removeOne() {
-    SILInstruction *instruction = worklist.pop_back_val();
-    worklistMap.erase(instruction);
-    return instruction;
+  SILInstruction *pop_back_val() {
+    return worklist.pop_back_val().getValueOr(nullptr);
   }
 
   /// When an instruction has been simplified, add all of its users to the
   /// worklist, since additional simplifications of its users may have been
   /// exposed.
-  void addUsersToWorklist(ValueBase *instruction) {
-    for (auto *use : instruction->getUses())
-      add(use->getUser());
-  }
-
   void addUsersToWorklist(SILValue value) {
-    for (auto *use : value->getUses())
+    for (auto *use : value->getUses()) {
       add(use->getUser());
+    }
   }
 
   /// When an instruction has been simplified, add all of its users to the
   /// worklist, since additional simplifications of its users may have been
   /// exposed.
   void addUsersOfAllResultsToWorklist(SILInstruction *instruction) {
     for (auto result : instruction->getResults()) {
-      addUsersToWorklist(result);
+      addUsersToWorklist(&*result);
     }
   }
 
-  /// Check that the worklist is empty and nuke the backing store for the map if
-  /// it is large.
-  void zap() {
-    assert(worklistMap.empty() && "Worklist empty, but the map is not?");
+  /// Check that the worklist is empty and nuke the backing store if it is
+  /// large.
+  void resetChecked() {
+    assert(worklist.empty() && "Vector empty, but the map is not?");
 
-    // Do an explicit clear, this shrinks the map if needed.
-    worklistMap.clear();
+    // Do an explicit clear, shrinking the storage if needed.
+    worklist.clear();
   }
 };
 
+// TODO: This name is somewhat unpleasant.  Once the type is templated over its
+//       storage and renamed BlottableWorklist, this type will be
+//       SmallBlottableWorklist which is more reasonable.
+template <unsigned N>
+class SmallSILInstructionWorklist final
+    : public SILInstructionWorklist<
+          llvm::SmallVector<Optional<SILInstruction *>, N>,
+          // TODO: A DenseMap rather than a SmallDenseMap is used here to avoid
+          //       running into an upstream problem with the handling of grow()
+          //       when it results in rehashing and tombstone removal:
+          //
+          //       https://reviews.llvm.org/D56455
+          llvm::DenseMap<SILInstruction *, unsigned>> {
+public:
+  using VectorT = llvm::SmallVector<Optional<SILInstruction *>, N>;
+  using MapT = llvm::DenseMap<SILInstruction *, unsigned>;
+  SmallSILInstructionWorklist(const char *loggingName = "InstructionWorklist")
+      : SILInstructionWorklist<VectorT, MapT>(loggingName) {}
+};
+
 } // end namespace swift
diff --git a/include/swift/SILOptimizer/Analysis/LoopRegionAnalysis.h b/include/swift/SILOptimizer/Analysis/LoopRegionAnalysis.h
@@ -762,7 +762,7 @@ class LoopRegion {
 
   unsigned addSucc(LoopRegion *Successor) {
     assert(!isFunction() && "Functions cannot have successors");
-    return Succs.insert(SuccessorID(Successor->getID(), false));
+    return Succs.insert(SuccessorID(Successor->getID(), false)).first;
   }
 
   void replacePred(unsigned OldPredID, unsigned NewPredID) {
diff --git a/lib/SIL/SILInstructionWorklist.cpp b/lib/SIL/SILInstructionWorklist.cpp
@@ -15,24 +15,10 @@
 
 using namespace swift;
 
-void SILInstructionWorklist::add(SILInstruction *instruction) {
-  if (!worklistMap.insert(std::make_pair(instruction, worklist.size())).second)
-    return;
-
-  LLVM_DEBUG(llvm::dbgs() << loggingName << ": ADD: " << *instruction << '\n');
-  worklist.push_back(instruction);
-}
-
-void SILInstructionWorklist::addInitialGroup(ArrayRef<SILInstruction *> list) {
-  assert(worklist.empty() && "worklist must be empty to add initial group");
-  worklist.reserve(list.size() + 16);
-  worklistMap.reserve(list.size());
-  LLVM_DEBUG(llvm::dbgs() << loggingName << ": ADDING: " << list.size()
-                          << " instrs to worklist\n");
-  while (!list.empty()) {
-    SILInstruction *instruction = list.back();
-    list = list.slice(0, list.size() - 1);
-    worklistMap.insert(std::make_pair(instruction, worklist.size()));
-    worklist.push_back(instruction);
-  }
+void SILInstructionWorklistBase::withDebugStream(
+    std::function<void(llvm::raw_ostream &stream, const char *loggingName)>
+        perform) {
+#ifndef NDEBUG
+  LLVM_DEBUG(perform(llvm::dbgs(), loggingName));
+#endif
 }
diff --git a/lib/SILOptimizer/SILCombiner/SILCombine.cpp b/lib/SILOptimizer/SILCombiner/SILCombine.cpp
diff --git a/lib/SILOptimizer/SILCombiner/SILCombiner.h b/lib/SILOptimizer/SILCombiner/SILCombiner.h

Original file line number	Diff line number	Diff line change
`@@ -762,7 +762,7 @@ class LoopRegion {`
`762`	`762`
`763`	`763`	`unsigned addSucc(LoopRegion *Successor) {`
`764`	`764`	`assert(!isFunction() && "Functions cannot have successors");`
`765`		`- return Succs.insert(SuccessorID(Successor->getID(), false));`
	`765`	`+ return Succs.insert(SuccessorID(Successor->getID(), false)).first;`
`766`	`766`	`}`
`767`	`767`
`768`	`768`	`void replacePred(unsigned OldPredID, unsigned NewPredID) {`