From c462faa8443ce59180e99fa8f051e1f8299fc445 Mon Sep 17 00:00:00 2001 From: Brian Favela Date: Wed, 28 Jun 2023 17:50:32 -0400 Subject: [PATCH] [NFC] More cherry picks in preparation of ReverseIterator changes (#5340) Continuing work from https://github.com/microsoft/DirectXShaderCompiler/pull/5329 Another 2 cherry picks. Bit bigger this time, but no changes required apart from the free->delete change as before --------- Co-authored-by: Matthias Braun Co-authored-by: Benjamin Kramer --- include/llvm/ADT/DenseMap.h | 90 ++++++++++++++++++--------- include/llvm/ADT/SmallPtrSet.h | 66 +++++++++++++------- lib/Support/SmallPtrSet.cpp | 109 ++++++++++++++++----------------- unittests/ADT/DenseMapTest.cpp | 10 +++ 4 files changed, 167 insertions(+), 108 deletions(-) diff --git a/include/llvm/ADT/DenseMap.h b/include/llvm/ADT/DenseMap.h index 29cf72d7da..2f991e60ec 100644 --- a/include/llvm/ADT/DenseMap.h +++ b/include/llvm/ADT/DenseMap.h @@ -167,30 +167,65 @@ class DenseMapBase : public DebugEpochBase { // If the key is already in the map, it returns false and doesn't update the // value. std::pair insert(const std::pair &KV) { + return try_emplace(KV.first, KV.second); + } + + // Inserts key,value pair into the map if the key isn't already in the map. + // If the key is already in the map, it returns false and doesn't update the + // value. + std::pair insert(std::pair &&KV) { + return try_emplace(std::move(KV.first), std::move(KV.second)); + } + + // Inserts key,value pair into the map if the key isn't already in the map. + // The value is constructed in-place if the key is not in the map, otherwise + // it is not moved. + template + std::pair try_emplace(KeyT &&Key, Ts &&... Args) { BucketT *TheBucket; - if (LookupBucketFor(KV.first, TheBucket)) + if (LookupBucketFor(Key, TheBucket)) return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true), false); // Already in map. // Otherwise, insert the new element. - TheBucket = InsertIntoBucket(KV.first, KV.second, TheBucket); + TheBucket = + InsertIntoBucket(TheBucket, std::move(Key), std::forward(Args)...); return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true), true); } // Inserts key,value pair into the map if the key isn't already in the map. - // If the key is already in the map, it returns false and doesn't update the - // value. - std::pair insert(std::pair &&KV) { + // The value is constructed in-place if the key is not in the map, otherwise + // it is not moved. + template + std::pair try_emplace(const KeyT &Key, Ts &&... Args) { + BucketT *TheBucket; + if (LookupBucketFor(Key, TheBucket)) + return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true), + false); // Already in map. + + // Otherwise, insert the new element. + TheBucket = InsertIntoBucket(TheBucket, Key, std::forward(Args)...); + return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true), + true); + } + + /// Alternate version of insert() which allows a different, and possibly + /// less expensive, key type. + /// The DenseMapInfo is responsible for supplying methods + /// getHashValue(LookupKeyT) and isEqual(LookupKeyT, KeyT) for each key + /// type used. + template + std::pair insert_as(std::pair &&KV, + const LookupKeyT &Val) { BucketT *TheBucket; - if (LookupBucketFor(KV.first, TheBucket)) + if (LookupBucketFor(Val, TheBucket)) return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true), false); // Already in map. // Otherwise, insert the new element. - TheBucket = InsertIntoBucket(std::move(KV.first), - std::move(KV.second), - TheBucket); + TheBucket = InsertIntoBucketWithLookup(TheBucket, std::move(KV.first), + std::move(KV.second), Val); return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true), true); } @@ -227,7 +262,7 @@ class DenseMapBase : public DebugEpochBase { if (LookupBucketFor(Key, TheBucket)) return *TheBucket; - return *InsertIntoBucket(Key, ValueT(), TheBucket); + return *InsertIntoBucket(TheBucket, Key); } ValueT &operator[](const KeyT &Key) { @@ -239,7 +274,7 @@ class DenseMapBase : public DebugEpochBase { if (LookupBucketFor(Key, TheBucket)) return *TheBucket; - return *InsertIntoBucket(std::move(Key), ValueT(), TheBucket); + return *InsertIntoBucket(TheBucket, std::move(Key)); } ValueT &operator[](KeyT &&Key) { @@ -396,34 +431,29 @@ class DenseMapBase : public DebugEpochBase { static_cast(this)->shrink_and_clear(); } + template + BucketT *InsertIntoBucket(BucketT *TheBucket, KeyArg &&Key, + ValueArgs &&... Values) { + TheBucket = InsertIntoBucketImpl(Key, Key, TheBucket); - BucketT *InsertIntoBucket(const KeyT &Key, const ValueT &Value, - BucketT *TheBucket) { - TheBucket = InsertIntoBucketImpl(Key, TheBucket); - - TheBucket->getFirst() = Key; - new (&TheBucket->getSecond()) ValueT(Value); + TheBucket->getFirst() = std::forward(Key); + ::new (&TheBucket->getSecond()) ValueT(std::forward(Values)...); return TheBucket; } - BucketT *InsertIntoBucket(const KeyT &Key, ValueT &&Value, - BucketT *TheBucket) { - TheBucket = InsertIntoBucketImpl(Key, TheBucket); - - TheBucket->getFirst() = Key; - new (&TheBucket->getSecond()) ValueT(std::move(Value)); - return TheBucket; - } - - BucketT *InsertIntoBucket(KeyT &&Key, ValueT &&Value, BucketT *TheBucket) { - TheBucket = InsertIntoBucketImpl(Key, TheBucket); + template + BucketT *InsertIntoBucketWithLookup(BucketT *TheBucket, KeyT &&Key, + ValueT &&Value, LookupKeyT &Lookup) { + TheBucket = InsertIntoBucketImpl(Key, Lookup, TheBucket); TheBucket->getFirst() = std::move(Key); - new (&TheBucket->getSecond()) ValueT(std::move(Value)); + ::new (&TheBucket->getSecond()) ValueT(std::move(Value)); return TheBucket; } - BucketT *InsertIntoBucketImpl(const KeyT &Key, BucketT *TheBucket) { + template + BucketT *InsertIntoBucketImpl(const KeyT &Key, const LookupKeyT &Lookup, + BucketT *TheBucket) { incrementEpoch(); // If the load of the hash table is more than 3/4, or if fewer than 1/8 of diff --git a/include/llvm/ADT/SmallPtrSet.h b/include/llvm/ADT/SmallPtrSet.h index c21d5d12af..2114db731d 100644 --- a/include/llvm/ADT/SmallPtrSet.h +++ b/include/llvm/ADT/SmallPtrSet.h @@ -57,36 +57,41 @@ class SmallPtrSetImplBase { /// CurArraySize - The allocated size of CurArray, always a power of two. unsigned CurArraySize; - // If small, this is # elts allocated consecutively - unsigned NumElements; + /// Number of elements in CurArray that contain a value or are a tombstone. + /// If small, all these elements are at the beginning of CurArray and the rest + /// is uninitialized. + unsigned NumNonEmpty; + /// Number of tombstones in CurArray. unsigned NumTombstones; // Helpers to copy and move construct a SmallPtrSet. SmallPtrSetImplBase(const void **SmallStorage, const SmallPtrSetImplBase &that); SmallPtrSetImplBase(const void **SmallStorage, unsigned SmallSize, - SmallPtrSetImplBase &&that); - explicit SmallPtrSetImplBase(const void **SmallStorage, unsigned SmallSize) : - SmallArray(SmallStorage), CurArray(SmallStorage), CurArraySize(SmallSize) { + SmallPtrSetImplBase &&that); + explicit SmallPtrSetImplBase(const void **SmallStorage, unsigned SmallSize) + : SmallArray(SmallStorage), CurArray(SmallStorage), + CurArraySize(SmallSize), NumNonEmpty(0), NumTombstones(0) { assert(SmallSize && (SmallSize & (SmallSize-1)) == 0 && "Initial size must be a power of two!"); - clear(); } ~SmallPtrSetImplBase(); public: typedef unsigned size_type; bool LLVM_ATTRIBUTE_UNUSED_RESULT empty() const { return size() == 0; } - size_type size() const { return NumElements; } + size_type size() const { return NumNonEmpty - NumTombstones; } void clear() { // If the capacity of the array is huge, and the # elements used is small, // shrink the array. - if (!isSmall() && NumElements*4 < CurArraySize && CurArraySize > 32) - return shrink_and_clear(); + if (!isSmall()) { + if (size() * 4 < CurArraySize && CurArraySize > 32) + return shrink_and_clear(); + // Fill the array with empty markers. + memset(CurArray, -1, CurArraySize * sizeof(void *)); + } - // Fill the array with empty markers. - memset(CurArray, -1, CurArraySize*sizeof(void*)); - NumElements = 0; + NumNonEmpty = 0; NumTombstones = 0; } @@ -98,21 +103,37 @@ class SmallPtrSetImplBase { return reinterpret_cast(-1); } + const void **EndPointer() const { + return isSmall() ? CurArray + NumNonEmpty : CurArray + CurArraySize; + } + /// insert_imp - This returns true if the pointer was new to the set, false if /// it was already in the set. This is hidden from the client so that the /// derived class can check that the right type of pointer is passed in. std::pair insert_imp(const void *Ptr) { if (isSmall()) { // Check to see if it is already in the set. - for (const void **APtr = SmallArray, **E = SmallArray+NumElements; - APtr != E; ++APtr) - if (*APtr == Ptr) + const void **LastTombstone = nullptr; + for (const void **APtr = SmallArray, **E = SmallArray + NumNonEmpty; + APtr != E; ++APtr) { + const void *Value = *APtr; + if (Value == Ptr) return std::make_pair(APtr, false); + if (Value == getTombstoneMarker()) + LastTombstone = APtr; + } + + // Did we find any tombstone marker? + if (LastTombstone != nullptr) { + *LastTombstone = Ptr; + --NumTombstones; + return std::make_pair(LastTombstone, true); + } // Nope, there isn't. If we stay small, just 'pushback' now. - if (NumElements < CurArraySize) { - SmallArray[NumElements++] = Ptr; - return std::make_pair(SmallArray + (NumElements - 1), true); + if (NumNonEmpty < CurArraySize) { + SmallArray[NumNonEmpty++] = Ptr; + return std::make_pair(SmallArray + (NumNonEmpty - 1), true); } // Otherwise, hit the big set case, which will call grow. } @@ -129,7 +150,7 @@ class SmallPtrSetImplBase { if (isSmall()) { // Linear search for the item. for (const void *const *APtr = SmallArray, - *const *E = SmallArray+NumElements; APtr != E; ++APtr) + *const *E = SmallArray + NumNonEmpty; APtr != E; ++APtr) if (*APtr == Ptr) return true; return false; @@ -287,7 +308,7 @@ class SmallPtrSetImpl : public SmallPtrSetImplBase { /// the element equal to Ptr. std::pair insert(PtrType Ptr) { auto p = insert_imp(PtrTraits::getAsVoidPointer(Ptr)); - return std::make_pair(iterator(p.first, CurArray + CurArraySize), p.second); + return std::make_pair(iterator(p.first, EndPointer()), p.second); } /// erase - If the set contains the specified pointer, remove it and return @@ -308,10 +329,11 @@ class SmallPtrSetImpl : public SmallPtrSetImplBase { } inline iterator begin() const { - return iterator(CurArray, CurArray+CurArraySize); + return iterator(CurArray, EndPointer()); } inline iterator end() const { - return iterator(CurArray+CurArraySize, CurArray+CurArraySize); + const void *const *End = EndPointer(); + return iterator(End, End); } }; diff --git a/lib/Support/SmallPtrSet.cpp b/lib/Support/SmallPtrSet.cpp index 56acf2723f..25c77b5919 100644 --- a/lib/Support/SmallPtrSet.cpp +++ b/lib/Support/SmallPtrSet.cpp @@ -25,8 +25,9 @@ void SmallPtrSetImplBase::shrink_and_clear() { delete[] CurArray; // HLSL Change: Use overridable operator delete // Reduce the number of buckets. - CurArraySize = NumElements > 16 ? 1 << (Log2_32_Ceil(NumElements) + 1) : 32; - NumElements = NumTombstones = 0; + unsigned Size = size(); + CurArraySize = Size > 16 ? 1 << (Log2_32_Ceil(Size) + 1) : 32; + NumNonEmpty = NumTombstones = 0; // Install the new array. Clear all the buckets to empty. CurArray = new const void*[CurArraySize]; // HLSL Change: Use overridable operator new @@ -36,11 +37,10 @@ void SmallPtrSetImplBase::shrink_and_clear() { std::pair SmallPtrSetImplBase::insert_imp_big(const void *Ptr) { - if (LLVM_UNLIKELY(NumElements * 4 >= CurArraySize * 3)) { + if (LLVM_UNLIKELY(size() * 4 >= CurArraySize * 3)) { // If more than 3/4 of the array is full, grow. - Grow(CurArraySize < 64 ? 128 : CurArraySize*2); - } else if (LLVM_UNLIKELY(CurArraySize - (NumElements + NumTombstones) < - CurArraySize / 8)) { + Grow(CurArraySize < 64 ? 128 : CurArraySize * 2); + } else if (LLVM_UNLIKELY(CurArraySize - NumNonEmpty < CurArraySize / 8)) { // If fewer of 1/8 of the array is empty (meaning that many are filled with // tombstones), rehash. Grow(CurArraySize); @@ -54,21 +54,21 @@ SmallPtrSetImplBase::insert_imp_big(const void *Ptr) { // Otherwise, insert it! if (*Bucket == getTombstoneMarker()) --NumTombstones; + else + ++NumNonEmpty; // Track density. *Bucket = Ptr; - ++NumElements; // Track density. return std::make_pair(Bucket, true); } bool SmallPtrSetImplBase::erase_imp(const void * Ptr) { if (isSmall()) { // Check to see if it is in the set. - for (const void **APtr = SmallArray, **E = SmallArray+NumElements; - APtr != E; ++APtr) + for (const void **APtr = CurArray, **E = CurArray + NumNonEmpty; APtr != E; + ++APtr) if (*APtr == Ptr) { // If it is in the set, replace this element. - *APtr = E[-1]; - E[-1] = getEmptyMarker(); - --NumElements; + *APtr = getTombstoneMarker(); + ++NumTombstones; return true; } @@ -81,7 +81,6 @@ bool SmallPtrSetImplBase::erase_imp(const void * Ptr) { // Set this as a tombstone. *Bucket = getTombstoneMarker(); - --NumElements; ++NumTombstones; return true; } @@ -116,10 +115,8 @@ const void * const *SmallPtrSetImplBase::FindBucketFor(const void *Ptr) const { /// Grow - Allocate a larger backing store for the buckets and move it over. /// void SmallPtrSetImplBase::Grow(unsigned NewSize) { - // Allocate at twice as many buckets, but at least 128. - unsigned OldSize = CurArraySize; - const void **OldBuckets = CurArray; + const void **OldEnd = EndPointer(); bool WasSmall = isSmall(); // Install the new array. Clear all the buckets to empty. @@ -127,28 +124,19 @@ void SmallPtrSetImplBase::Grow(unsigned NewSize) { assert(CurArray && "Failed to allocate memory?"); CurArraySize = NewSize; memset(CurArray, -1, NewSize*sizeof(void*)); - - // Copy over all the elements. - if (WasSmall) { - // Small sets store their elements in order. - for (const void **BucketPtr = OldBuckets, **E = OldBuckets+NumElements; - BucketPtr != E; ++BucketPtr) { - const void *Elt = *BucketPtr; + + // Copy over all valid entries. + for (const void **BucketPtr = OldBuckets; BucketPtr != OldEnd; ++BucketPtr) { + // Copy over the element if it is valid. + const void *Elt = *BucketPtr; + if (Elt != getTombstoneMarker() && Elt != getEmptyMarker()) *const_cast(FindBucketFor(Elt)) = const_cast(Elt); - } - } else { - // Copy over all valid entries. - for (const void **BucketPtr = OldBuckets, **E = OldBuckets+OldSize; - BucketPtr != E; ++BucketPtr) { - // Copy over the element if it is valid. - const void *Elt = *BucketPtr; - if (Elt != getTombstoneMarker() && Elt != getEmptyMarker()) - *const_cast(FindBucketFor(Elt)) = const_cast(Elt); - } - - delete[] OldBuckets; // HLSL Change: Use overridable operator delete - NumTombstones = 0; } + + if (!WasSmall) + delete [] OldBuckets; + NumNonEmpty -= NumTombstones; + NumTombstones = 0; } SmallPtrSetImplBase::SmallPtrSetImplBase(const void **SmallStorage, @@ -210,9 +198,9 @@ void SmallPtrSetImplBase::CopyHelper(const SmallPtrSetImplBase &RHS) { CurArraySize = RHS.CurArraySize; // Copy over the contents from the other set - memcpy(CurArray, RHS.CurArray, sizeof(void*)*CurArraySize); - - NumElements = RHS.NumElements; + std::copy(RHS.CurArray, RHS.EndPointer(), CurArray); + + NumNonEmpty = RHS.NumNonEmpty; NumTombstones = RHS.NumTombstones; } @@ -230,7 +218,7 @@ void SmallPtrSetImplBase::MoveHelper(unsigned SmallSize, if (RHS.isSmall()) { // Copy a small RHS rather than moving. CurArray = SmallArray; - memcpy(CurArray, RHS.CurArray, sizeof(void*)*RHS.CurArraySize); + std::copy(RHS.CurArray, RHS.CurArray + RHS.NumNonEmpty, CurArray); } else { CurArray = RHS.CurArray; RHS.CurArray = RHS.SmallArray; @@ -238,13 +226,13 @@ void SmallPtrSetImplBase::MoveHelper(unsigned SmallSize, // Copy the rest of the trivial members. CurArraySize = RHS.CurArraySize; - NumElements = RHS.NumElements; + NumNonEmpty = RHS.NumNonEmpty; NumTombstones = RHS.NumTombstones; // Make the RHS small and empty. RHS.CurArraySize = SmallSize; assert(RHS.CurArray == RHS.SmallArray); - RHS.NumElements = 0; + RHS.NumNonEmpty = 0; RHS.NumTombstones = 0; } @@ -255,7 +243,7 @@ void SmallPtrSetImplBase::swap(SmallPtrSetImplBase &RHS) { if (!this->isSmall() && !RHS.isSmall()) { std::swap(this->CurArray, RHS.CurArray); std::swap(this->CurArraySize, RHS.CurArraySize); - std::swap(this->NumElements, RHS.NumElements); + std::swap(this->NumNonEmpty, RHS.NumNonEmpty); std::swap(this->NumTombstones, RHS.NumTombstones); return; } @@ -265,37 +253,46 @@ void SmallPtrSetImplBase::swap(SmallPtrSetImplBase &RHS) { // If only RHS is small, copy the small elements into LHS and move the pointer // from LHS to RHS. if (!this->isSmall() && RHS.isSmall()) { - std::copy(RHS.SmallArray, RHS.SmallArray+RHS.CurArraySize, - this->SmallArray); - std::swap(this->NumElements, RHS.NumElements); - std::swap(this->CurArraySize, RHS.CurArraySize); + assert(RHS.CurArray == RHS.SmallArray); + std::copy(RHS.CurArray, RHS.CurArray + RHS.NumNonEmpty, this->SmallArray); + std::swap(RHS.CurArraySize, this->CurArraySize); + std::swap(this->NumNonEmpty, RHS.NumNonEmpty); + std::swap(this->NumTombstones, RHS.NumTombstones); RHS.CurArray = this->CurArray; - RHS.NumTombstones = this->NumTombstones; this->CurArray = this->SmallArray; - this->NumTombstones = 0; return; } // If only LHS is small, copy the small elements into RHS and move the pointer // from RHS to LHS. if (this->isSmall() && !RHS.isSmall()) { - std::copy(this->SmallArray, this->SmallArray+this->CurArraySize, + assert(this->CurArray == this->SmallArray); + std::copy(this->CurArray, this->CurArray + this->NumNonEmpty, RHS.SmallArray); - std::swap(RHS.NumElements, this->NumElements); std::swap(RHS.CurArraySize, this->CurArraySize); + std::swap(RHS.NumNonEmpty, this->NumNonEmpty); + std::swap(RHS.NumTombstones, this->NumTombstones); this->CurArray = RHS.CurArray; - this->NumTombstones = RHS.NumTombstones; RHS.CurArray = RHS.SmallArray; - RHS.NumTombstones = 0; return; } // Both a small, just swap the small elements. assert(this->isSmall() && RHS.isSmall()); - assert(this->CurArraySize == RHS.CurArraySize); - std::swap_ranges(this->SmallArray, this->SmallArray+this->CurArraySize, + unsigned MinNonEmpty = std::min(this->NumNonEmpty, RHS.NumNonEmpty); + std::swap_ranges(this->SmallArray, this->SmallArray + MinNonEmpty, RHS.SmallArray); - std::swap(this->NumElements, RHS.NumElements); + if (this->NumNonEmpty > MinNonEmpty) { + std::copy(this->SmallArray + MinNonEmpty, + this->SmallArray + this->NumNonEmpty, + RHS.SmallArray + MinNonEmpty); + } else { + std::copy(RHS.SmallArray + MinNonEmpty, RHS.SmallArray + RHS.NumNonEmpty, + this->SmallArray + MinNonEmpty); + } + assert(this->CurArraySize == RHS.CurArraySize); + std::swap(this->NumNonEmpty, RHS.NumNonEmpty); + std::swap(this->NumTombstones, RHS.NumTombstones); } SmallPtrSetImplBase::~SmallPtrSetImplBase() { diff --git a/unittests/ADT/DenseMapTest.cpp b/unittests/ADT/DenseMapTest.cpp index 90625e99f2..b25bea87aa 100644 --- a/unittests/ADT/DenseMapTest.cpp +++ b/unittests/ADT/DenseMapTest.cpp @@ -422,4 +422,14 @@ TEST(DenseMapCustomTest, SmallDenseMapGrowTest) { EXPECT_TRUE(map.find(32) == map.end()); } +TEST(DenseMapCustomTest, TryEmplaceTest) { + DenseMap> Map; + std::unique_ptr P(new int(2)); + auto Try1 = Map.try_emplace(0, new int(1)); + EXPECT_TRUE(Try1.second); + auto Try2 = Map.try_emplace(0, std::move(P)); + EXPECT_FALSE(Try2.second); + EXPECT_EQ(Try1.first, Try2.first); + EXPECT_NE(nullptr, P); +} }