[SmallPtrSet] Remove SmallArray member (NFC) #118099
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-adt Author: Nikita Popov (nikic) ChangesCurrently SmallPtrSet stores CurArray and SmallArray, with the former pointing to the latter in small representation. Instead, only use CurArray and a separate IsSmall boolean. Most of the implementation doesn't need the separate SmallArray member -- we only need it for copy/move/swap operations, where we may switch back from large to small representation. In those cases, we explicitly pass down the pointer to SmallStorage. This reduces the size of SmallPtrSet and improves compile-time: https://llvm-compile-time-tracker.com/compare.php?from=352f8688d0ca250c9e8774321f6c3bcd4298cc09&to=f8b23930d54ecd06ec32e77aa90f4b774fe31841&stat=instructions:u Full diff: https://github.com/llvm/llvm-project/pull/118099.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ADT/SmallPtrSet.h b/llvm/include/llvm/ADT/SmallPtrSet.h
index 1fc2318342ae78..1018114a307a4d 100644
--- a/llvm/include/llvm/ADT/SmallPtrSet.h
+++ b/llvm/include/llvm/ADT/SmallPtrSet.h
@@ -53,10 +53,7 @@ class SmallPtrSetImplBase : public DebugEpochBase {
friend class SmallPtrSetIteratorImpl;
protected:
- /// SmallArray - Points to a fixed size set of buckets, used in 'small mode'.
- const void **SmallArray;
- /// CurArray - This is the current set of buckets. If equal to SmallArray,
- /// then the set is in 'small mode'.
+ /// The current set of buckets, in either small or big representation.
const void **CurArray;
/// CurArraySize - The allocated size of CurArray, always a power of two.
unsigned CurArraySize;
@@ -67,16 +64,18 @@ class SmallPtrSetImplBase : public DebugEpochBase {
unsigned NumNonEmpty;
/// Number of tombstones in CurArray.
unsigned NumTombstones;
+ /// Whether the set is in small representation.
+ bool IsSmall;
// Helpers to copy and move construct a SmallPtrSet.
SmallPtrSetImplBase(const void **SmallStorage,
const SmallPtrSetImplBase &that);
SmallPtrSetImplBase(const void **SmallStorage, unsigned SmallSize,
- SmallPtrSetImplBase &&that);
+ const void **RHSSmallStorage, SmallPtrSetImplBase &&that);
explicit SmallPtrSetImplBase(const void **SmallStorage, unsigned SmallSize)
- : SmallArray(SmallStorage), CurArray(SmallStorage),
- CurArraySize(SmallSize), NumNonEmpty(0), NumTombstones(0) {
+ : CurArray(SmallStorage), CurArraySize(SmallSize), NumNonEmpty(0),
+ NumTombstones(0), IsSmall(true) {
assert(SmallSize && (SmallSize & (SmallSize-1)) == 0 &&
"Initial size must be a power of two!");
}
@@ -150,7 +149,7 @@ class SmallPtrSetImplBase : public DebugEpochBase {
std::pair<const void *const *, bool> insert_imp(const void *Ptr) {
if (isSmall()) {
// Check to see if it is already in the set.
- for (const void **APtr = SmallArray, **E = SmallArray + NumNonEmpty;
+ for (const void **APtr = CurArray, **E = CurArray + NumNonEmpty;
APtr != E; ++APtr) {
const void *Value = *APtr;
if (Value == Ptr)
@@ -159,9 +158,9 @@ class SmallPtrSetImplBase : public DebugEpochBase {
// Nope, there isn't. If we stay small, just 'pushback' now.
if (NumNonEmpty < CurArraySize) {
- SmallArray[NumNonEmpty++] = Ptr;
+ CurArray[NumNonEmpty++] = Ptr;
incrementEpoch();
- return std::make_pair(SmallArray + (NumNonEmpty - 1), true);
+ return std::make_pair(CurArray + (NumNonEmpty - 1), true);
}
// Otherwise, hit the big set case, which will call grow.
}
@@ -174,10 +173,10 @@ class SmallPtrSetImplBase : public DebugEpochBase {
/// in.
bool erase_imp(const void * Ptr) {
if (isSmall()) {
- for (const void **APtr = SmallArray, **E = SmallArray + NumNonEmpty;
+ for (const void **APtr = CurArray, **E = CurArray + NumNonEmpty;
APtr != E; ++APtr) {
if (*APtr == Ptr) {
- *APtr = SmallArray[--NumNonEmpty];
+ *APtr = CurArray[--NumNonEmpty];
incrementEpoch();
return true;
}
@@ -203,8 +202,9 @@ class SmallPtrSetImplBase : public DebugEpochBase {
const void *const * find_imp(const void * Ptr) const {
if (isSmall()) {
// Linear search for the item.
- for (const void *const *APtr = SmallArray,
- *const *E = SmallArray + NumNonEmpty; APtr != E; ++APtr)
+ for (const void *const *APtr = CurArray, *const *E =
+ CurArray + NumNonEmpty;
+ APtr != E; ++APtr)
if (*APtr == Ptr)
return APtr;
return EndPointer();
@@ -216,7 +216,7 @@ class SmallPtrSetImplBase : public DebugEpochBase {
return EndPointer();
}
- bool isSmall() const { return CurArray == SmallArray; }
+ bool isSmall() const { return IsSmall; }
private:
std::pair<const void *const *, bool> insert_imp_big(const void *Ptr);
@@ -231,14 +231,17 @@ class SmallPtrSetImplBase : public DebugEpochBase {
protected:
/// swap - Swaps the elements of two sets.
/// Note: This method assumes that both sets have the same small size.
- void swap(SmallPtrSetImplBase &RHS);
+ void swap(const void **SmallStorage, const void **RHSSmallStorage,
+ SmallPtrSetImplBase &RHS);
- void CopyFrom(const SmallPtrSetImplBase &RHS);
- void MoveFrom(unsigned SmallSize, SmallPtrSetImplBase &&RHS);
+ void CopyFrom(const void **SmallStorage, const SmallPtrSetImplBase &RHS);
+ void MoveFrom(const void **SmallStorage, unsigned SmallSize,
+ const void **RHSSmallStorage, SmallPtrSetImplBase &&RHS);
private:
/// Code shared by MoveFrom() and move constructor.
- void MoveHelper(unsigned SmallSize, SmallPtrSetImplBase &&RHS);
+ void MoveHelper(const void **SmallStorage, unsigned SmallSize,
+ const void **RHSSmallStorage, SmallPtrSetImplBase &&RHS);
/// Code shared by CopyFrom() and copy constructor.
void CopyHelper(const SmallPtrSetImplBase &RHS);
};
@@ -401,7 +404,7 @@ class SmallPtrSetImpl : public SmallPtrSetImplBase {
bool remove_if(UnaryPredicate P) {
bool Removed = false;
if (isSmall()) {
- const void **APtr = SmallArray, **E = SmallArray + NumNonEmpty;
+ const void **APtr = CurArray, **E = CurArray + NumNonEmpty;
while (APtr != E) {
PtrType Ptr = PtrTraits::getFromVoidPointer(const_cast<void *>(*APtr));
if (P(Ptr)) {
@@ -526,7 +529,8 @@ class SmallPtrSet : public SmallPtrSetImpl<PtrType> {
SmallPtrSet() : BaseT(SmallStorage, SmallSizePowTwo) {}
SmallPtrSet(const SmallPtrSet &that) : BaseT(SmallStorage, that) {}
SmallPtrSet(SmallPtrSet &&that)
- : BaseT(SmallStorage, SmallSizePowTwo, std::move(that)) {}
+ : BaseT(SmallStorage, SmallSizePowTwo, that.SmallStorage,
+ std::move(that)) {}
template<typename It>
SmallPtrSet(It I, It E) : BaseT(SmallStorage, SmallSizePowTwo) {
@@ -541,14 +545,15 @@ class SmallPtrSet : public SmallPtrSetImpl<PtrType> {
SmallPtrSet<PtrType, SmallSize> &
operator=(const SmallPtrSet<PtrType, SmallSize> &RHS) {
if (&RHS != this)
- this->CopyFrom(RHS);
+ this->CopyFrom(SmallStorage, RHS);
return *this;
}
SmallPtrSet<PtrType, SmallSize> &
operator=(SmallPtrSet<PtrType, SmallSize> &&RHS) {
if (&RHS != this)
- this->MoveFrom(SmallSizePowTwo, std::move(RHS));
+ this->MoveFrom(SmallStorage, SmallSizePowTwo, RHS.SmallStorage,
+ std::move(RHS));
return *this;
}
@@ -561,7 +566,7 @@ class SmallPtrSet : public SmallPtrSetImpl<PtrType> {
/// swap - Swaps the elements of two sets.
void swap(SmallPtrSet<PtrType, SmallSize> &RHS) {
- SmallPtrSetImplBase::swap(RHS);
+ SmallPtrSetImplBase::swap(SmallStorage, RHS.SmallStorage, RHS);
}
};
diff --git a/llvm/lib/Support/SmallPtrSet.cpp b/llvm/lib/Support/SmallPtrSet.cpp
index 1bec911f39b13e..74662f3a3461db 100644
--- a/llvm/lib/Support/SmallPtrSet.cpp
+++ b/llvm/lib/Support/SmallPtrSet.cpp
@@ -134,17 +134,17 @@ void SmallPtrSetImplBase::Grow(unsigned NewSize) {
free(OldBuckets);
NumNonEmpty -= NumTombstones;
NumTombstones = 0;
+ IsSmall = false;
}
SmallPtrSetImplBase::SmallPtrSetImplBase(const void **SmallStorage,
const SmallPtrSetImplBase &that) {
- SmallArray = SmallStorage;
-
- // If we're becoming small, prepare to insert into our stack space
- if (that.isSmall()) {
- CurArray = SmallArray;
- // Otherwise, allocate new heap space (unless we were the same size)
+ IsSmall = that.isSmall();
+ if (IsSmall) {
+ // If we're becoming small, prepare to insert into our stack space
+ CurArray = SmallStorage;
} else {
+ // Otherwise, allocate new heap space (unless we were the same size)
CurArray = (const void**)safe_malloc(sizeof(void*) * that.CurArraySize);
}
@@ -154,12 +154,13 @@ SmallPtrSetImplBase::SmallPtrSetImplBase(const void **SmallStorage,
SmallPtrSetImplBase::SmallPtrSetImplBase(const void **SmallStorage,
unsigned SmallSize,
+ const void **RHSSmallStorage,
SmallPtrSetImplBase &&that) {
- SmallArray = SmallStorage;
- MoveHelper(SmallSize, std::move(that));
+ MoveHelper(SmallStorage, SmallSize, RHSSmallStorage, std::move(that));
}
-void SmallPtrSetImplBase::CopyFrom(const SmallPtrSetImplBase &RHS) {
+void SmallPtrSetImplBase::CopyFrom(const void **SmallStorage,
+ const SmallPtrSetImplBase &RHS) {
assert(&RHS != this && "Self-copy should be handled by the caller.");
if (isSmall() && RHS.isSmall())
@@ -170,8 +171,9 @@ void SmallPtrSetImplBase::CopyFrom(const SmallPtrSetImplBase &RHS) {
if (RHS.isSmall()) {
if (!isSmall())
free(CurArray);
- CurArray = SmallArray;
- // Otherwise, allocate new heap space (unless we were the same size)
+ CurArray = SmallStorage;
+ IsSmall = true;
+ // Otherwise, allocate new heap space (unless we were the same size)
} else if (CurArraySize != RHS.CurArraySize) {
if (isSmall())
CurArray = (const void**)safe_malloc(sizeof(void*) * RHS.CurArraySize);
@@ -180,6 +182,7 @@ void SmallPtrSetImplBase::CopyFrom(const SmallPtrSetImplBase &RHS) {
sizeof(void*) * RHS.CurArraySize);
CurArray = T;
}
+ IsSmall = false;
}
CopyHelper(RHS);
@@ -196,39 +199,46 @@ void SmallPtrSetImplBase::CopyHelper(const SmallPtrSetImplBase &RHS) {
NumTombstones = RHS.NumTombstones;
}
-void SmallPtrSetImplBase::MoveFrom(unsigned SmallSize,
+void SmallPtrSetImplBase::MoveFrom(const void **SmallStorage,
+ unsigned SmallSize,
+ const void **RHSSmallStorage,
SmallPtrSetImplBase &&RHS) {
if (!isSmall())
free(CurArray);
- MoveHelper(SmallSize, std::move(RHS));
+ MoveHelper(SmallStorage, SmallSize, RHSSmallStorage, std::move(RHS));
}
-void SmallPtrSetImplBase::MoveHelper(unsigned SmallSize,
+void SmallPtrSetImplBase::MoveHelper(const void **SmallStorage,
+ unsigned SmallSize,
+ const void **RHSSmallStorage,
SmallPtrSetImplBase &&RHS) {
assert(&RHS != this && "Self-move should be handled by the caller.");
if (RHS.isSmall()) {
// Copy a small RHS rather than moving.
- CurArray = SmallArray;
+ CurArray = SmallStorage;
std::copy(RHS.CurArray, RHS.CurArray + RHS.NumNonEmpty, CurArray);
} else {
CurArray = RHS.CurArray;
- RHS.CurArray = RHS.SmallArray;
+ RHS.CurArray = RHSSmallStorage;
}
// Copy the rest of the trivial members.
CurArraySize = RHS.CurArraySize;
NumNonEmpty = RHS.NumNonEmpty;
NumTombstones = RHS.NumTombstones;
+ IsSmall = RHS.IsSmall;
// Make the RHS small and empty.
RHS.CurArraySize = SmallSize;
- assert(RHS.CurArray == RHS.SmallArray);
RHS.NumNonEmpty = 0;
RHS.NumTombstones = 0;
+ RHS.IsSmall = true;
}
-void SmallPtrSetImplBase::swap(SmallPtrSetImplBase &RHS) {
+void SmallPtrSetImplBase::swap(const void **SmallStorage,
+ const void **RHSSmallStorage,
+ SmallPtrSetImplBase &RHS) {
if (this == &RHS) return;
// We can only avoid copying elements if neither set is small.
@@ -245,42 +255,42 @@ 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()) {
- assert(RHS.CurArray == RHS.SmallArray);
- std::copy(RHS.CurArray, RHS.CurArray + RHS.NumNonEmpty, this->SmallArray);
+ std::copy(RHS.CurArray, RHS.CurArray + RHS.NumNonEmpty, SmallStorage);
std::swap(RHS.CurArraySize, this->CurArraySize);
std::swap(this->NumNonEmpty, RHS.NumNonEmpty);
std::swap(this->NumTombstones, RHS.NumTombstones);
RHS.CurArray = this->CurArray;
- this->CurArray = this->SmallArray;
+ RHS.IsSmall = false;
+ this->CurArray = SmallStorage;
+ this->IsSmall = true;
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()) {
- assert(this->CurArray == this->SmallArray);
std::copy(this->CurArray, this->CurArray + this->NumNonEmpty,
- RHS.SmallArray);
+ RHSSmallStorage);
std::swap(RHS.CurArraySize, this->CurArraySize);
std::swap(RHS.NumNonEmpty, this->NumNonEmpty);
std::swap(RHS.NumTombstones, this->NumTombstones);
this->CurArray = RHS.CurArray;
- RHS.CurArray = RHS.SmallArray;
+ this->IsSmall = false;
+ RHS.CurArray = RHSSmallStorage;
+ RHS.IsSmall = true;
return;
}
// Both a small, just swap the small elements.
assert(this->isSmall() && RHS.isSmall());
unsigned MinNonEmpty = std::min(this->NumNonEmpty, RHS.NumNonEmpty);
- std::swap_ranges(this->SmallArray, this->SmallArray + MinNonEmpty,
- RHS.SmallArray);
+ std::swap_ranges(this->CurArray, this->CurArray + MinNonEmpty, RHS.CurArray);
if (this->NumNonEmpty > MinNonEmpty) {
- std::copy(this->SmallArray + MinNonEmpty,
- this->SmallArray + this->NumNonEmpty,
- RHS.SmallArray + MinNonEmpty);
+ std::copy(this->CurArray + MinNonEmpty, this->CurArray + this->NumNonEmpty,
+ RHS.CurArray + MinNonEmpty);
} else {
- std::copy(RHS.SmallArray + MinNonEmpty, RHS.SmallArray + RHS.NumNonEmpty,
- this->SmallArray + MinNonEmpty);
+ std::copy(RHS.CurArray + MinNonEmpty, RHS.CurArray + RHS.NumNonEmpty,
+ this->CurArray + MinNonEmpty);
}
assert(this->CurArraySize == RHS.CurArraySize);
std::swap(this->NumNonEmpty, RHS.NumNonEmpty);
|
kuhar
approved these changes
Nov 29, 2024
MaskRay
approved these changes
Nov 29, 2024
llvm/include/llvm/ADT/SmallPtrSet.h
Outdated
|
|
||
| void CopyFrom(const SmallPtrSetImplBase &RHS); | ||
| void MoveFrom(unsigned SmallSize, SmallPtrSetImplBase &&RHS); | ||
| void CopyFrom(const void **SmallStorage, const SmallPtrSetImplBase &RHS); |
Member
There was a problem hiding this comment.
Optional: as you are changing the signature, you can rename MoveFrom to moveFrom to improve the naming consistency.
Currently SmallPtrSet stores CurArray and SmallArray, with the former pointing to the latter in small representation. Instead, only use CurArray and a separate IsSmall boolean. Most of the implementation doesn't need the separate SmallArray member -- we only need it for copy/move/swap operations, where we may switch back from large to small representation. In those cases, we explicitly pass down the pointer to SmallStorage.
nikic
force-pushed
the
smallptrset-small-array
branch
from
51d73db to
9bed44e
Compare
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/11343 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/35/builds/4178 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/4/builds/3923 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/41/builds/3799 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently SmallPtrSet stores CurArray and SmallArray, with the former pointing to the latter in small representation. Instead, only use CurArray and a separate IsSmall boolean.
Most of the implementation doesn't need the separate SmallArray member -- we only need it for copy/move/swap operations, where we may switch back from large to small representation. In those cases, we explicitly pass down the pointer to SmallStorage.
This reduces the size of SmallPtrSet and improves compile-time: https://llvm-compile-time-tracker.com/compare.php?from=352f8688d0ca250c9e8774321f6c3bcd4298cc09&to=f8b23930d54ecd06ec32e77aa90f4b774fe31841&stat=instructions:u