[llvm-branch-commits] [llvm] [ADT] Use perfect forwarding in SmallSet::insert() (PR #108590)
Victor Campos via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Fri Sep 20 05:23:41 PDT 2024
https://github.com/vhscampos updated https://github.com/llvm/llvm-project/pull/108590
>From a2438ce9a61d8e80aa32fa58ca5368a64deacd52 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 1/4] [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 630c98504261aa..d5f64e4f20f854 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -139,10 +139,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.
@@ -163,13 +159,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
@@ -181,7 +171,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) {
@@ -206,11 +196,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;
}
@@ -234,19 +225,12 @@ 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();
- }
};
/// If this set is of pointer values, transparently switch over to using
>From e0295a14694c5bba65a873914eb700b201e94b6a 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 2/4] [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 d5f64e4f20f854..c8641111eda8f8 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -165,26 +165,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); }
- 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) {
- 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>
@@ -231,6 +215,31 @@ class SmallSet {
private:
bool isSmall() const { return Set.empty(); }
+
+ 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);
+ }
+
+ 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) {
+ 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 47c7444d4391d3bcbead2bb86a09ae051e0c4802 Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Tue, 17 Sep 2024 13:11:00 +0100
Subject: [PATCH 3/4] Add test for SmallSet::insert perfect forwarding
---
llvm/unittests/ADT/SmallSetTest.cpp | 34 +++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/llvm/unittests/ADT/SmallSetTest.cpp b/llvm/unittests/ADT/SmallSetTest.cpp
index b50b368ae66361..0fb20b19df9254 100644
--- a/llvm/unittests/ADT/SmallSetTest.cpp
+++ b/llvm/unittests/ADT/SmallSetTest.cpp
@@ -41,6 +41,40 @@ TEST(SmallSetTest, Insert) {
EXPECT_EQ(0u, s1.count(4));
}
+TEST(SmallSetTest, InsertPerfectFwd) {
+ struct Value {
+ int Key;
+ bool Moved;
+
+ Value(int Key) : Key(Key), Moved(false) {}
+ Value(const Value &) = default;
+ Value(Value &&Other) : Key(Other.Key), Moved(false) { Other.Moved = true; }
+ bool operator==(const Value &Other) const { return Key == Other.Key; }
+ bool operator<(const Value &Other) const { return Key < Other.Key; }
+ };
+
+ {
+ SmallSet<Value, 4> S;
+ Value V1(1), V2(2);
+
+ S.insert(V1);
+ EXPECT_EQ(V1.Moved, false);
+
+ S.insert(std::move(V2));
+ EXPECT_EQ(V2.Moved, true);
+ }
+ {
+ SmallSet<Value, 1> S;
+ Value V1(1), V2(2);
+
+ S.insert(V1);
+ EXPECT_EQ(V1.Moved, false);
+
+ S.insert(std::move(V2));
+ EXPECT_EQ(V2.Moved, true);
+ }
+}
+
TEST(SmallSetTest, Grow) {
SmallSet<int, 4> s1;
>From a2eb49509529eb0a04346e50f64aa071b91e8c93 Mon Sep 17 00:00:00 2001
From: Victor Campos <victor.campos at arm.com>
Date: Tue, 17 Sep 2024 13:16:24 +0100
Subject: [PATCH 4/4] Style changes
---
llvm/include/llvm/ADT/SmallSet.h | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/llvm/include/llvm/ADT/SmallSet.h b/llvm/include/llvm/ADT/SmallSet.h
index c8641111eda8f8..2beaf2b92c028b 100644
--- a/llvm/include/llvm/ADT/SmallSet.h
+++ b/llvm/include/llvm/ADT/SmallSet.h
@@ -222,23 +222,22 @@ class SmallSet {
"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);
+ return {const_iterator(I), Inserted};
}
- 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 (auto I = llvm::find(Vector, V);
+ I != Vector.end()) // Don't reinsert if it already exists.
+ return {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);
+ return {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);
+ return {const_iterator(Set.insert(std::forward<ArgType>(V)).first), true};
}
};
More information about the llvm-branch-commits
mailing list