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

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: chandlerc, hfinkel.
Herald added subscribers: llvm-commits, kristina, dexonsmith.

Fixes issue encountered in D56362 <https://reviews.llvm.org/D56362>, where I tried to use an `SmallSetVector<Instruction*, 128>` with an excessively large number of inline elements. This triggers an "Must allocate more buckets than are inline" assertion inside `allocateBuckets()` under certain usage patterns.

The issue is as follows: The `grow()` method is used either to grow the map, or to rehash it and remove tombstones. The latter is done if the fraction of empty (non-used, non-tombstone) elements is below 1/8. In this case `grow()` is invoked with the current number of buckets.

This is currently incorrectly handled for dense maps using the small rep. The current implementation will switch them over to the large rep, which violates the invariant that the large rep is only used if there are more than InlineBuckets buckets.

This patch fixes the issue by staying in the small rep and only moving the buckets. Alternatively, if we do want to switch to the large rep in this case, would be to relax the assertion in `allocateBuckets()`.


Repository:
  rL LLVM

https://reviews.llvm.org/D56455

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


Index: unittests/ADT/DenseMapTest.cpp
===================================================================
--- unittests/ADT/DenseMapTest.cpp
+++ unittests/ADT/DenseMapTest.cpp
@@ -590,6 +590,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: include/llvm/ADT/DenseMap.h
===================================================================
--- include/llvm/ADT/DenseMap.h
+++ include/llvm/ADT/DenseMap.h
@@ -1027,13 +1027,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);
@@ -1056,10 +1053,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.180736.patch
Type: text/x-patch
Size: 2329 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190108/775de166/attachment.bin>