[llvm] [ADT] Series of improvements to SmallSet (PR #104611)
Victor Campos via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 16 09:22:48 PDT 2024
https://github.com/vhscampos created https://github.com/llvm/llvm-project/pull/104611
This PR introduces a series of improvements to the SmallSet data structure.
- Use universal reference/perfect forwarding in `SmallSet::insert` interface.
- Introduction of several new API functions:
- Constructor that takes pair of iterators.
- Constructor that takes a range.
- Constructor that takes an initializer list.
- Copy constructor.
- Move constructor.
- Copy assignment operator.
- Move assignment operator.
- Style warnings fixes.
- Modernisation of code using newer C++ constructs.
>From f99687cdd97f554b7ddec9bc7016f96b747c46b4 Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Fri, 9 Aug 2024 13:51:52 +0100
Subject: [PATCH 1/6] [ADT] Fix naming of `isSmall` field in SmallSetIterator
clang-tidy previously brought up a warning about `isSmall`. According to
the naming rules, it should be named `IsSmall`.
This has been fixed.
---
llvm/include/llvm/ADT/SmallSet.h | 36 ++++++++++++++++----------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index a16e8ac6f07552..24d136cdf7bc51 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -46,24 +46,24 @@ class SmallSetIterator
VecIterTy VecIter;
};
- bool isSmall;
+ bool IsSmall;
public:
- SmallSetIterator(SetIterTy SetIter) : SetIter(SetIter), isSmall(false) {}
+ SmallSetIterator(SetIterTy SetIter) : SetIter(SetIter), IsSmall(false) {}
- SmallSetIterator(VecIterTy VecIter) : VecIter(VecIter), isSmall(true) {}
+ SmallSetIterator(VecIterTy VecIter) : VecIter(VecIter), IsSmall(true) {}
// Spell out destructor, copy/move constructor and assignment operators for
// MSVC STL, where set<T>::const_iterator is not trivially copy constructible.
~SmallSetIterator() {
- if (isSmall)
+ if (IsSmall)
VecIter.~VecIterTy();
else
SetIter.~SetIterTy();
}
- SmallSetIterator(const SmallSetIterator &Other) : isSmall(Other.isSmall) {
- if (isSmall)
+ SmallSetIterator(const SmallSetIterator &Other) : IsSmall(Other.IsSmall) {
+ if (IsSmall)
VecIter = Other.VecIter;
else
// Use placement new, to make sure SetIter is properly constructed, even
@@ -71,8 +71,8 @@ class SmallSetIterator
new (&SetIter) SetIterTy(Other.SetIter);
}
- SmallSetIterator(SmallSetIterator &&Other) : isSmall(Other.isSmall) {
- if (isSmall)
+ SmallSetIterator(SmallSetIterator &&Other) : IsSmall(Other.IsSmall) {
+ if (IsSmall)
VecIter = std::move(Other.VecIter);
else
// Use placement new, to make sure SetIter is properly constructed, even
@@ -83,11 +83,11 @@ class SmallSetIterator
SmallSetIterator& operator=(const SmallSetIterator& Other) {
// Call destructor for SetIter, so it gets properly destroyed if it is
// not trivially destructible in case we are setting VecIter.
- if (!isSmall)
+ if (!IsSmall)
SetIter.~SetIterTy();
- isSmall = Other.isSmall;
- if (isSmall)
+ IsSmall = Other.IsSmall;
+ if (IsSmall)
VecIter = Other.VecIter;
else
new (&SetIter) SetIterTy(Other.SetIter);
@@ -97,11 +97,11 @@ class SmallSetIterator
SmallSetIterator& operator=(SmallSetIterator&& Other) {
// Call destructor for SetIter, so it gets properly destroyed if it is
// not trivially destructible in case we are setting VecIter.
- if (!isSmall)
+ if (!IsSmall)
SetIter.~SetIterTy();
- isSmall = Other.isSmall;
- if (isSmall)
+ IsSmall = Other.IsSmall;
+ if (IsSmall)
VecIter = std::move(Other.VecIter);
else
new (&SetIter) SetIterTy(std::move(Other.SetIter));
@@ -109,22 +109,22 @@ class SmallSetIterator
}
bool operator==(const SmallSetIterator &RHS) const {
- if (isSmall != RHS.isSmall)
+ if (IsSmall != RHS.IsSmall)
return false;
- if (isSmall)
+ if (IsSmall)
return VecIter == RHS.VecIter;
return SetIter == RHS.SetIter;
}
SmallSetIterator &operator++() { // Preincrement
- if (isSmall)
+ if (IsSmall)
VecIter++;
else
SetIter++;
return *this;
}
- const T &operator*() const { return isSmall ? *VecIter : *SetIter; }
+ const T &operator*() const { return IsSmall ? *VecIter : *SetIter; }
};
/// SmallSet - This maintains a set of unique values, optimizing for the case
>From f77f36b3cf11686b9582710b5ef09d951140fe22 Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Fri, 9 Aug 2024 13:53:46 +0100
Subject: [PATCH 2/6] [ADT] Use pre-increment in SmallSetIterator
Replace post-increment by pre-increment in the underlying iterator of
SmallSetIterator.
---
llvm/include/llvm/ADT/SmallSet.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index 24d136cdf7bc51..9fae8e4c01bcde 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -118,9 +118,9 @@ class SmallSetIterator
SmallSetIterator &operator++() { // Preincrement
if (IsSmall)
- VecIter++;
+ ++VecIter;
else
- SetIter++;
+ ++SetIter;
return *this;
}
>From b08c3e4ba26828fe52890de43dcf105bc7abd7c2 Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Fri, 9 Aug 2024 13:55:38 +0100
Subject: [PATCH 3/6] [ADT] Remove unnecessary else branch in SmallSet::erase
---
llvm/include/llvm/ADT/SmallSet.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index 9fae8e4c01bcde..630c98504261aa 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -167,9 +167,8 @@ class SmallSet {
if (isSmall()) {
// Since the collection is small, just do a linear search.
return vfind(V) == Vector.end() ? 0 : 1;
- } else {
- return Set.count(V);
}
+ return Set.count(V);
}
/// insert - Insert an element into the set if it isn't already there.
>From 005e60cc85d63a44e6f1798a08d123ed9c057d2d Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Fri, 9 Aug 2024 13:57:42 +0100
Subject: [PATCH 4/6] [ADT] Use perfect forwarding in SmallSet::insert()
Previously this method took arguments by const-ref. This patch changes
the implementation to take perfectly forwarded arguments in the form of
a universal reference. Now, the insertion method will take advantage of
arguments passed as rvalue, potentially leading to performance
improvements.
---
llvm/include/llvm/ADT/SmallSet.h | 47 +++++++++++++++++++-------------
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index 630c98504261aa..36c4f6385cd45a 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -175,26 +175,10 @@ class SmallSet {
/// Returns a pair. The first value of it is an iterator to the inserted
/// element or the existing element in the set. The second value is true
/// if the element is inserted (it was not in the set before).
- std::pair<const_iterator, bool> insert(const T &V) {
- if (!isSmall()) {
- auto [I, Inserted] = Set.insert(V);
- return std::make_pair(const_iterator(I), Inserted);
- }
+ std::pair<const_iterator, bool> insert(const T &V) { return insertImpl(V); }
- VIterator I = vfind(V);
- if (I != Vector.end()) // Don't reinsert if it already exists.
- return std::make_pair(const_iterator(I), false);
- if (Vector.size() < N) {
- Vector.push_back(V);
- return std::make_pair(const_iterator(std::prev(Vector.end())), true);
- }
-
- // Otherwise, grow from vector to set.
- while (!Vector.empty()) {
- Set.insert(Vector.back());
- Vector.pop_back();
- }
- return std::make_pair(const_iterator(Set.insert(V).first), true);
+ std::pair<const_iterator, bool> insert(T &&V) {
+ return insertImpl(std::move(V));
}
template <typename IterT>
@@ -247,6 +231,31 @@ class SmallSet {
return I;
return Vector.end();
}
+
+ template <typename ArgType>
+ std::pair<const_iterator, bool> insertImpl(ArgType &&V) {
+ static_assert(std::is_convertible_v<ArgType, T>,
+ "ArgType must be convertible to T!");
+ if (!isSmall()) {
+ auto [I, Inserted] = Set.insert(std::forward<ArgType>(V));
+ return std::make_pair(const_iterator(I), Inserted);
+ }
+
+ VIterator I = vfind(V);
+ if (I != Vector.end()) // Don't reinsert if it already exists.
+ return std::make_pair(const_iterator(I), false);
+ if (Vector.size() < N) {
+ Vector.push_back(std::forward<ArgType>(V));
+ return std::make_pair(const_iterator(std::prev(Vector.end())), true);
+ }
+
+ // Otherwise, grow from vector to set.
+ Set.insert(std::make_move_iterator(Vector.begin()),
+ std::make_move_iterator(Vector.end()));
+ Vector.clear();
+ return std::make_pair(
+ const_iterator(Set.insert(std::forward<ArgType>(V)).first), true);
+ }
};
/// If this set is of pointer values, transparently switch over to using
>From 552cfa4fc1629caa6e4c006de7d541a9f41f3660 Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Fri, 9 Aug 2024 14:00:32 +0100
Subject: [PATCH 5/6] [ADT] Add more useful methods to SmallSet API
This patch adds useful methods to the SmallSet API:
- Constructor that takes pair of iterators.
- Constructor that takes a range.
- Constructor that takes an initializer list.
- Copy constructor.
- Move constructor.
- Copy assignment operator.
- Move assignment operator.
---
llvm/include/llvm/ADT/SmallSet.h | 21 ++++++++--
llvm/unittests/ADT/SmallSetTest.cpp | 60 +++++++++++++++++++++++++++++
2 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index 36c4f6385cd45a..607f57a85cdef4 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -14,14 +14,13 @@
#ifndef LLVM_ADT_SMALLSET_H
#define LLVM_ADT_SMALLSET_H
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/iterator.h"
-#include "llvm/Support/Compiler.h"
-#include "llvm/Support/type_traits.h"
#include <cstddef>
#include <functional>
+#include <initializer_list>
#include <set>
#include <type_traits>
#include <utility>
@@ -155,6 +154,22 @@ class SmallSet {
using const_iterator = SmallSetIterator<T, N, C>;
SmallSet() = default;
+ SmallSet(const SmallSet &) = default;
+ SmallSet(SmallSet &&) = default;
+
+ template <typename IterT> explicit SmallSet(IterT Begin, IterT End) {
+ this->insert(Begin, End);
+ }
+
+ template <typename RangeT>
+ explicit SmallSet(const iterator_range<RangeT> &R) {
+ this->insert(R.begin(), R.end());
+ }
+
+ SmallSet(std::initializer_list<T> L) { this->insert(L.begin(), L.end()); }
+
+ SmallSet &operator=(const SmallSet &) = default;
+ SmallSet &operator=(SmallSet &&) = default;
[[nodiscard]] bool empty() const { return Vector.empty() && Set.empty(); }
diff --git a/llvm/unittests/ADT/SmallSetTest.cpp b/llvm/unittests/ADT/SmallSetTest.cpp
index b50b368ae66361..ced1ba5dce34d8 100644
--- a/llvm/unittests/ADT/SmallSetTest.cpp
+++ b/llvm/unittests/ADT/SmallSetTest.cpp
@@ -17,6 +17,66 @@
using namespace llvm;
+TEST(SmallSetTest, ConstructorIteratorPair) {
+ auto L = {1, 2, 3, 4, 5};
+ SmallSet<int, 4> S(std::begin(L), std::end(L));
+ for (int Value : L)
+ EXPECT_TRUE(S.contains(Value));
+}
+
+TEST(SmallSet, ConstructorRange) {
+ auto L = {1, 2, 3, 4, 5};
+
+ SmallSet<int, 4> S(llvm::make_range(std::begin(L), std::end(L)));
+ for (int Value : L)
+ EXPECT_TRUE(S.contains(Value));
+}
+
+TEST(SmallSet, ConstructorInitializerList) {
+ auto L = {1, 2, 3, 4, 5};
+ SmallSet<int, 4> S = {1, 2, 3, 4, 5};
+ for (int Value : L)
+ EXPECT_TRUE(S.contains(Value));
+}
+
+TEST(SmallSet, CopyConstructor) {
+ SmallSet<int, 4> S = {1, 2, 3};
+ SmallSet<int, 4> T = S;
+
+ EXPECT_EQ(S, T);
+}
+
+TEST(SmallSet, MoveConstructor) {
+ auto L = {1, 2, 3};
+ SmallSet<int, 4> S = L;
+ SmallSet<int, 4> T = std::move(S);
+
+ EXPECT_TRUE(T.size() == L.size());
+ for (int Value : L) {
+ EXPECT_TRUE(T.contains(Value));
+ }
+}
+
+TEST(SmallSet, CopyAssignment) {
+ SmallSet<int, 4> S = {1, 2, 3};
+ SmallSet<int, 4> T;
+ T = S;
+
+ EXPECT_EQ(S, T);
+}
+
+TEST(SmallSet, MoveAssignment) {
+ auto L = {1, 2, 3};
+ SmallSet<int, 4> S = L;
+ SmallSet<int, 4> T;
+ T = std::move(S);
+
+ EXPECT_TRUE(T.size() == L.size());
+ for (int Value : L) {
+ EXPECT_TRUE(T.contains(Value));
+ }
+}
+
TEST(SmallSetTest, Insert) {
SmallSet<int, 4> s1;
>From aecb8278850a8667a719ebc2d7e4ae1f4ac76fdf Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Fri, 9 Aug 2024 13:56:31 +0100
Subject: [PATCH 6/6] [ADT] Use range-based helper functions in SmallSet
Replace code that relies on iterators by LLVM helper functions that take
ranges. This makes the code simpler and more readable.
---
llvm/include/llvm/ADT/SmallSet.h | 36 +++++++++-----------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index 607f57a85cdef4..7898b7b11eb212 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -138,10 +138,6 @@ class SmallSet {
SmallVector<T, N> Vector;
std::set<T, C> Set;
- using VIterator = typename SmallVector<T, N>::const_iterator;
- using SIterator = typename std::set<T, C>::const_iterator;
- using mutable_iterator = typename SmallVector<T, N>::iterator;
-
// In small mode SmallPtrSet uses linear search for the elements, so it is
// not a good idea to choose this value too high. You may consider using a
// DenseSet<> instead if you expect many elements in the set.
@@ -178,13 +174,7 @@ class SmallSet {
}
/// count - Return 1 if the element is in the set, 0 otherwise.
- size_type count(const T &V) const {
- if (isSmall()) {
- // Since the collection is small, just do a linear search.
- return vfind(V) == Vector.end() ? 0 : 1;
- }
- return Set.count(V);
- }
+ size_type count(const T &V) const { return contains(V) ? 1 : 0; }
/// insert - Insert an element into the set if it isn't already there.
/// Returns a pair. The first value of it is an iterator to the inserted
@@ -205,11 +195,12 @@ class SmallSet {
bool erase(const T &V) {
if (!isSmall())
return Set.erase(V);
- for (mutable_iterator I = Vector.begin(), E = Vector.end(); I != E; ++I)
- if (*I == V) {
- Vector.erase(I);
- return true;
- }
+
+ auto It = llvm::find(Vector, V);
+ if (It != Vector.end()) {
+ Vector.erase(It);
+ return true;
+ }
return false;
}
@@ -233,20 +224,13 @@ class SmallSet {
/// Check if the SmallSet contains the given element.
bool contains(const T &V) const {
if (isSmall())
- return vfind(V) != Vector.end();
- return Set.find(V) != Set.end();
+ return llvm::is_contained(Vector, V);
+ return llvm::is_contained(Set, V);
}
private:
bool isSmall() const { return Set.empty(); }
- VIterator vfind(const T &V) const {
- for (VIterator I = Vector.begin(), E = Vector.end(); I != E; ++I)
- if (*I == V)
- return I;
- return Vector.end();
- }
-
template <typename ArgType>
std::pair<const_iterator, bool> insertImpl(ArgType &&V) {
static_assert(std::is_convertible_v<ArgType, T>,
@@ -256,7 +240,7 @@ class SmallSet {
return std::make_pair(const_iterator(I), Inserted);
}
- VIterator I = vfind(V);
+ auto I = llvm::find(Vector, V);
if (I != Vector.end()) // Don't reinsert if it already exists.
return std::make_pair(const_iterator(I), false);
if (Vector.size() < N) {
More information about the llvm-commits
mailing list