Skip to content

Commit ad8d0a1

Browse files
[ADT] Fix the initial size calculation of SmallDenseMap (#158458)
The initial size calculation of SmallDenseMap is strange in several ways: - SmallDenseMap(unsigned) seems to want to take the number of initial buckets as far as I can tell from the variable name NumInitBuckets. In contrast, DenseMap(unsigned) seems to want to take the number of initial entries as far as I can tell from the comment: /// Create a DenseMap with an optional \p InitialReserve that guarantee that /// this number of elements can be inserted in the map without grow() - SmallDenseMap(unsigned) uses llvm::bit_ceil to obtain a power of two. SmallDenseMap(I, E) uses NextPowerOf2 to obtain a power of two. - Presumably, the init() call is to ensure that we won't call grow() while populating the initial elements [I, E). However, NextPowerOf2(std::distance(I, E)) does not ensure that a rehash won't happen. For example, if the number of initial elements is 50, we need 128 buckets, but NextPowerOf2(std::distance(I, E)) would return 64. This patch fixes all these inconsistencies by teaching SmallDenseMap::init to call BaseT::getMinBucketToReserveForEntries just like DenseMap::init. With this patch, all constructors of SmallDenseMap are textually identical to their respective counterparts in DenseMap.
1 parent 3f26f1a commit ad8d0a1

File tree

2 files changed

+74
-6
lines changed

2 files changed

+74
-6
lines changed

llvm/include/llvm/ADT/DenseMap.h

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -887,10 +887,8 @@ class SmallDenseMap
887887
AlignedCharArrayUnion<BucketT[InlineBuckets], LargeRep> storage;
888888

889889
public:
890-
explicit SmallDenseMap(unsigned NumInitBuckets = 0) {
891-
if (NumInitBuckets > InlineBuckets)
892-
NumInitBuckets = llvm::bit_ceil(NumInitBuckets);
893-
init(NumInitBuckets);
890+
explicit SmallDenseMap(unsigned NumElementsToReservre = 0) {
891+
init(NumElementsToReservre);
894892
}
895893

896894
SmallDenseMap(const SmallDenseMap &other) : BaseT() {
@@ -905,7 +903,7 @@ class SmallDenseMap
905903

906904
template <typename InputIt>
907905
SmallDenseMap(const InputIt &I, const InputIt &E) {
908-
init(NextPowerOf2(std::distance(I, E)));
906+
init(std::distance(I, E));
909907
this->insert(I, E);
910908
}
911909

@@ -1017,7 +1015,8 @@ class SmallDenseMap
10171015
this->BaseT::copyFrom(other);
10181016
}
10191017

1020-
void init(unsigned InitBuckets) {
1018+
void init(unsigned InitNumEntries) {
1019+
auto InitBuckets = BaseT::getMinBucketToReserveForEntries(InitNumEntries);
10211020
Small = true;
10221021
if (InitBuckets > InlineBuckets) {
10231022
Small = false;

llvm/unittests/ADT/DenseMapTest.cpp

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -962,4 +962,73 @@ TEST(DenseMapCustomTest, PairPrinting) {
962962
EXPECT_EQ(R"({ (1, "one"), (2, "two") })", ::testing::PrintToString(Map));
963963
}
964964

965+
TEST(DenseMapCustomTest, InitSize) {
966+
constexpr unsigned ElemSize = sizeof(std::pair<int *, int>);
967+
968+
{
969+
DenseMap<int *, int> Map;
970+
EXPECT_EQ(ElemSize * 0U, Map.getMemorySize());
971+
}
972+
{
973+
DenseMap<int *, int> Map(0);
974+
EXPECT_EQ(ElemSize * 0U, Map.getMemorySize());
975+
}
976+
{
977+
DenseMap<int *, int> Map(1);
978+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
979+
}
980+
{
981+
DenseMap<int *, int> Map(2);
982+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
983+
}
984+
{
985+
DenseMap<int *, int> Map(3);
986+
EXPECT_EQ(ElemSize * 8U, Map.getMemorySize());
987+
}
988+
{
989+
int A, B;
990+
DenseMap<int *, int> Map = {{&A, 1}, {&B, 2}};
991+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
992+
}
993+
{
994+
int A, B, C;
995+
DenseMap<int *, int> Map = {{&A, 1}, {&B, 2}, {&C, 3}};
996+
EXPECT_EQ(ElemSize * 8U, Map.getMemorySize());
997+
}
998+
}
999+
1000+
TEST(SmallDenseMapCustomTest, InitSize) {
1001+
constexpr unsigned ElemSize = sizeof(std::pair<int *, int>);
1002+
{
1003+
SmallDenseMap<int *, int> Map;
1004+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
1005+
}
1006+
{
1007+
SmallDenseMap<int *, int> Map(0);
1008+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
1009+
}
1010+
{
1011+
SmallDenseMap<int *, int> Map(1);
1012+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
1013+
}
1014+
{
1015+
SmallDenseMap<int *, int> Map(2);
1016+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
1017+
}
1018+
{
1019+
SmallDenseMap<int *, int> Map(3);
1020+
EXPECT_EQ(ElemSize * 8U, Map.getMemorySize());
1021+
}
1022+
{
1023+
int A, B;
1024+
SmallDenseMap<int *, int> Map = {{&A, 1}, {&B, 2}};
1025+
EXPECT_EQ(ElemSize * 4U, Map.getMemorySize());
1026+
}
1027+
{
1028+
int A, B, C;
1029+
SmallDenseMap<int *, int> Map = {{&A, 1}, {&B, 2}, {&C, 3}};
1030+
EXPECT_EQ(ElemSize * 8U, Map.getMemorySize());
1031+
}
1032+
}
1033+
9651034
} // namespace

0 commit comments

Comments
 (0)