[PATCH] D56455: [ADT] Fix SmallDenseMap assertion with large InlineBuckets

[Nikita Popov via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe593fe15f78: [ADT] Fix SmallDenseMap assertion with large InlineBuckets (authored by nikic).

Changed prior to commit:
  https://reviews.llvm.org/D56455?vs=180736&id=233438#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56455/new/

https://reviews.llvm.org/D56455

Files:
  llvm/include/llvm/ADT/DenseMap.h
  llvm/unittests/ADT/DenseMapTest.cpp


Index: llvm/unittests/ADT/DenseMapTest.cpp
===================================================================
--- llvm/unittests/ADT/DenseMapTest.cpp
+++ llvm/unittests/ADT/DenseMapTest.cpp
@@ -580,6 +580,24 @@
   EXPECT_TRUE(map.find(32) == map.end());
 }
 
+TEST(DenseMapCustomTest, LargeSmallDenseMapCompaction) {
+  SmallDenseMap<unsigned, unsigned, 128, ContiguousDenseMapInfo> map;
+  // Fill to < 3/4 load.
+  for (unsigned i = 0; i < 95; ++i)
+    map[i] = i;
+  // And erase, leaving behind tombstones.
+  for (unsigned i = 0; i < 95; ++i)
+    map.erase(i);
+  // Fill further, so that less than 1/8 are empty, but still below 3/4 load.
+  for (unsigned i = 95; i < 128; ++i)
+    map[i] = i;
+
+  EXPECT_EQ(33u, map.size());
+  // Similar to the previous test, check for a non-existing element, as an
+  // indirect check that tombstones have been removed.
+  EXPECT_TRUE(map.find(0) == map.end());
+}
+
 TEST(DenseMapCustomTest, TryEmplaceTest) {
   DenseMap<int, std::unique_ptr<int>> Map;
   std::unique_ptr<int> P(new int(2));
Index: llvm/include/llvm/ADT/DenseMap.h
===================================================================
--- llvm/include/llvm/ADT/DenseMap.h
+++ llvm/include/llvm/ADT/DenseMap.h
@@ -1006,13 +1006,10 @@
   }
 
   void grow(unsigned AtLeast) {
-    if (AtLeast >= InlineBuckets)
+    if (AtLeast > InlineBuckets)
       AtLeast = std::max<unsigned>(64, NextPowerOf2(AtLeast-1));
 
     if (Small) {
-      if (AtLeast < InlineBuckets)
-        return; // Nothing to do.
-
       // First move the inline buckets into a temporary storage.
       AlignedCharArrayUnion<BucketT[InlineBuckets]> TmpStorage;
       BucketT *TmpBegin = reinterpret_cast<BucketT *>(TmpStorage.buffer);
@@ -1035,10 +1032,13 @@
         P->getFirst().~KeyT();
       }
 
-      // Now make this map use the large rep, and move all the entries back
-      // into it.
-      Small = false;
-      new (getLargeRep()) LargeRep(allocateBuckets(AtLeast));
+      // AtLeast == InlineBuckets can happen if there are many tombstones,
+      // and grow() is used to remove them. Usually we always switch to the
+      // large rep here.
+      if (AtLeast > InlineBuckets) {
+        Small = false;
+        new (getLargeRep()) LargeRep(allocateBuckets(AtLeast));
+      }
       this->moveFromOldBuckets(TmpBegin, TmpEnd);
       return;
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56455.233438.patch
Type: text/x-patch
Size: 2359 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191211/b8de1bb6/attachment.bin>