[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
Tue Sep 17 07:44:11 PDT 2024
https://github.com/vhscampos updated https://github.com/llvm/llvm-project/pull/108590
>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 1/3] [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 2/3] 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 3/3] 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