[ADT] Simplify DenseMapIterator with std::reverse_iterator (NFC) (llvm#157389)

DenseMapIterator has two tasks:

- iterate the buckets in the requested direction
- skip the empty and tombstone buckets

These tasks are intertwined in the current implementation.

This patch cleans up DenseMapIterator by separating the two tasks.
Specifically, we introduce a private middleman iterator type called
BucketItTy.  This is the same as the pointer-based iterator in the
forward direction, but it becomes std::reverse_iterator<pointer>
otherwise.  Now, the user-facing iterator iterates over BucketItTy
while skipping the empty and tombstone buckets.  This way,
AdvancePastEmptyBuckets always calls BucketItTy::operator++.  If the
reverse iteration is requested, the underlying raw pointer gets
decremented, but that logic is hidden behind
std::reverse_iterator<pointer>::operator++.

As a result, we can remove RetreatPastEmptyBuckets and a couple of
calls to shouldReverseIterate.

Here is a data point.  A couple of months ago, we were calling
shouldReverseIterate from 18 places in DenseMap.h.  That's down to 5.
This patch reduces it further down to 3.

Author: kazutakahirata

llvm/include/llvm/ADT/DenseMap.h

@@ -1184,10 +1184,14 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
   using iterator_category = std::forward_iterator_tag;
 
 private:
-  pointer Ptr = nullptr;
-  pointer End = nullptr;
+  using BucketItTy =
+      std::conditional_t<shouldReverseIterate<KeyT>(),
+                         std::reverse_iterator<pointer>, pointer>;
 
-  DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch)
+  BucketItTy Ptr = {};
+  BucketItTy End = {};
+
+  DenseMapIterator(BucketItTy Pos, BucketItTy E, const DebugEpochBase &Epoch)
       : DebugEpochBase::HandleBase(&Epoch), Ptr(Pos), End(E) {
     assert(isHandleInSync() && "invalid construction!");
   }
@@ -1201,29 +1205,24 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
     // empty buckets.
     if (IsEmpty)
       return makeEnd(Buckets, Epoch);
-    if (shouldReverseIterate<KeyT>()) {
-      DenseMapIterator Iter(Buckets.end(), Buckets.begin(), Epoch);
-      Iter.RetreatPastEmptyBuckets();
-      return Iter;
-    }
-    DenseMapIterator Iter(Buckets.begin(), Buckets.end(), Epoch);
+    auto R = maybeReverse(Buckets);
+    DenseMapIterator Iter(R.begin(), R.end(), Epoch);
     Iter.AdvancePastEmptyBuckets();
     return Iter;
   }
 
   static DenseMapIterator makeEnd(iterator_range<pointer> Buckets,
                                   const DebugEpochBase &Epoch) {
-    if (shouldReverseIterate<KeyT>())
-      return DenseMapIterator(Buckets.begin(), Buckets.begin(), Epoch);
-    return DenseMapIterator(Buckets.end(), Buckets.end(), Epoch);
+    auto R = maybeReverse(Buckets);
+    return DenseMapIterator(R.end(), R.end(), Epoch);
   }
 
   static DenseMapIterator makeIterator(pointer P,
                                        iterator_range<pointer> Buckets,
                                        const DebugEpochBase &Epoch) {
-    if (shouldReverseIterate<KeyT>())
-      return DenseMapIterator(P + 1, Buckets.begin(), Epoch);
-    return DenseMapIterator(P, Buckets.end(), Epoch);
+    auto R = maybeReverse(Buckets);
+    constexpr int Offset = shouldReverseIterate<KeyT>() ? 1 : 0;
+    return DenseMapIterator(BucketItTy(P + Offset), R.end(), Epoch);
   }
 
   // Converting ctor from non-const iterators to const iterators. SFINAE'd out
@@ -1238,16 +1237,16 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
   reference operator*() const {
     assert(isHandleInSync() && "invalid iterator access!");
     assert(Ptr != End && "dereferencing end() iterator");
-    if (shouldReverseIterate<KeyT>())
-      return Ptr[-1];
     return *Ptr;
   }
   pointer operator->() const { return &operator*(); }
 
   friend bool operator==(const DenseMapIterator &LHS,
                          const DenseMapIterator &RHS) {
-    assert((!LHS.Ptr || LHS.isHandleInSync()) && "handle not in sync!");
-    assert((!RHS.Ptr || RHS.isHandleInSync()) && "handle not in sync!");
+    assert((!LHS.getEpochAddress() || LHS.isHandleInSync()) &&
+           "handle not in sync!");
+    assert((!RHS.getEpochAddress() || RHS.isHandleInSync()) &&
+           "handle not in sync!");
     assert(LHS.getEpochAddress() == RHS.getEpochAddress() &&
            "comparing incomparable iterators!");
     return LHS.Ptr == RHS.Ptr;
@@ -1261,11 +1260,6 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
   inline DenseMapIterator &operator++() { // Preincrement
     assert(isHandleInSync() && "invalid iterator access!");
     assert(Ptr != End && "incrementing end() iterator");
-    if (shouldReverseIterate<KeyT>()) {
-      --Ptr;
-      RetreatPastEmptyBuckets();
-      return *this;
-    }
     ++Ptr;
     AdvancePastEmptyBuckets();
     return *this;
@@ -1288,14 +1282,11 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
       ++Ptr;
   }
 
-  void RetreatPastEmptyBuckets() {
-    assert(Ptr >= End);
-    const KeyT Empty = KeyInfoT::getEmptyKey();
-    const KeyT Tombstone = KeyInfoT::getTombstoneKey();
-
-    while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
-                          KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))
-      --Ptr;
+  static auto maybeReverse(iterator_range<pointer> Range) {
+    if constexpr (shouldReverseIterate<KeyT>())
+      return reverse(Range);
+    else
+      return Range;
   }
 };