[EquivalenceClasses] Introduce erase member function #134660
Conversation
Introduce 'erase(const ElemTy &V)' member function to allow the deletion of a certain value from EquivClasses. This is essential for certain scenarios that require modifying the contents of EquivClasses. This path also incidentally fixes a problem of inaccurate leader setting when EquivClasses unions two classes. This problem only arises in the presence of erase function.
|
@llvm/pr-subscribers-llvm-adt Author: donald chen (cxy-1993) ChangesIntroduce 'erase(const ElemTy &V)' member function to allow the deletion of a certain value from EquivClasses. This is essential for certain scenarios that require modifying the contents of EquivClasses. This path also incidentally fixes a problem of inaccurate leader setting when EquivClasses unions two classes. This problem only arises in the presence of erase function. Full diff: https://github.com/llvm/llvm-project/pull/134660.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ADT/EquivalenceClasses.h b/llvm/include/llvm/ADT/EquivalenceClasses.h
index e0a7af9421c35..e2e6b626993ef 100644
--- a/llvm/include/llvm/ADT/EquivalenceClasses.h
+++ b/llvm/include/llvm/ADT/EquivalenceClasses.h
@@ -220,6 +220,40 @@ template <class ElemTy> class EquivalenceClasses {
return *ECV;
}
+ /// erase - Erase a value from the union/find set, return if erase succeed.
+ bool erase(const ElemTy &V) {
+ if (!TheMapping.contains(V)) return false;
+ const ECValue *cur = TheMapping[V];
+ const ECValue *next = cur->getNext();
+ if (cur->isLeader()) {
+ if (next) {
+ next->Leader = cur->Leader;
+ auto nn = next->Next;
+ next->Next = (const ECValue*)((intptr_t)nn | (intptr_t)1);
+ }
+ } else {
+ const ECValue *leader = findLeader(V).Node;
+ const ECValue *pre = leader;
+ while (pre->getNext() != cur) {
+ pre = pre->getNext();
+ }
+ if (!next) {
+ pre->Next = nullptr;
+ leader->Leader = pre;
+ } else {
+ pre->Next = (const ECValue*)((intptr_t)next | (intptr_t)pre->isLeader());
+ next->Leader = pre;
+ }
+ }
+ TheMapping.erase(V);
+ for (auto I = Members.begin(); I != Members.end(); I++) {
+ if (*I == cur) {
+ Members.erase(I);
+ break;
+ }
+ }
+ return true;
+ }
/// findLeader - Given a value in the set, return a member iterator for the
/// equivalence class it is in. This does the path-compression part that
/// makes union-find "union findy". This returns an end iterator if the value
@@ -247,7 +281,9 @@ template <class ElemTy> class EquivalenceClasses {
// Otherwise, this is a real union operation. Set the end of the L1 list to
// point to the L2 leader node.
const ECValue &L1LV = *L1.Node, &L2LV = *L2.Node;
- L1LV.getEndOfList()->setNext(&L2LV);
+ const ECValue *L1LastV = L1LV.getEndOfList();
+
+ L1LastV->setNext(&L2LV);
// Update L1LV's end of list pointer.
L1LV.Leader = L2LV.getEndOfList();
@@ -255,8 +291,8 @@ template <class ElemTy> class EquivalenceClasses {
// Clear L2's leader flag:
L2LV.Next = L2LV.getNext();
- // L2's leader is now L1.
- L2LV.Leader = &L1LV;
+ // L2's leader is now last value of L1.
+ L2LV.Leader = L1LastV;
return L1;
}
diff --git a/llvm/unittests/ADT/EquivalenceClassesTest.cpp b/llvm/unittests/ADT/EquivalenceClassesTest.cpp
index ff243f51102fb..3d5c48eb8e1b6 100644
--- a/llvm/unittests/ADT/EquivalenceClassesTest.cpp
+++ b/llvm/unittests/ADT/EquivalenceClassesTest.cpp
@@ -59,6 +59,55 @@ TEST(EquivalenceClassesTest, SimpleMerge2) {
EXPECT_TRUE(EqClasses.isEquivalent(i, j));
}
+TEST(EquivalenceClassesTest, SimpleErase1) {
+ EquivalenceClasses<int> EqClasses;
+ // Check that erase head success.
+ // After erase A from (A, B ,C, D), <B, C, D> belong to one set.
+ EqClasses.unionSets(0, 1);
+ EqClasses.unionSets(2, 3);
+ EqClasses.unionSets(0, 2);
+ EXPECT_TRUE(EqClasses.erase(0));
+ for (int i = 1; i < 4; ++i)
+ for (int j = 1; j < 4; ++j)
+ EXPECT_TRUE(EqClasses.isEquivalent(i, j));
+}
+
+TEST(EquivalenceClassesTest, SimpleErase2) {
+ EquivalenceClasses<int> EqClasses;
+ // Check that erase tail success.
+ // After erase D from (A, B ,C, D), <A, B, C> belong to one set.
+ EqClasses.unionSets(0, 1);
+ EqClasses.unionSets(2, 3);
+ EqClasses.unionSets(0, 2);
+ EXPECT_TRUE(EqClasses.erase(3));
+ for (int i = 0; i < 3; ++i)
+ for (int j = 0; j < 3; ++j)
+ EXPECT_TRUE(EqClasses.isEquivalent(i, j));
+}
+
+TEST(EquivalenceClassesTest, SimpleErase3) {
+ EquivalenceClasses<int> EqClasses;
+ // Check that erase a value in the middle success.
+ // After erase B from (A, B ,C, D), <A, C, D> belong to one set.
+ EqClasses.unionSets(0, 1);
+ EqClasses.unionSets(2, 3);
+ EqClasses.unionSets(0, 2);
+ EXPECT_TRUE(EqClasses.erase(1));
+ for (int i = 0; i < 3; ++i)
+ for (int j = 0; j < 3; ++j)
+ EXPECT_TRUE(EqClasses.isEquivalent(i, j) ^ ((i == 1) ^ (j == 1)));
+}
+
+TEST(EquivalenceClassesTest, SimpleErase4) {
+ EquivalenceClasses<int> EqClasses;
+ // Check that erase a single class success.
+ EqClasses.insert(0);
+ EXPECT_TRUE(EqClasses.getNumClasses() == 1);
+ EXPECT_TRUE(EqClasses.erase(0));
+ EXPECT_TRUE(EqClasses.getNumClasses() == 0);
+ EXPECT_FALSE(EqClasses.erase(1));
+}
+
TEST(EquivalenceClassesTest, TwoSets) {
EquivalenceClasses<int> EqClasses;
// Form sets of odd and even numbers, check that we split them into these
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
What code is intended to use the new behavior? I'd like to better understand why a value's equivalence may change?
cxy-1993
force-pushed
the
erase-equivalent
branch
from
8fcec6d to
86db294
Compare
cxy-1993
force-pushed
the
erase-equivalent
branch
from
86db294 to
acf94ab
Compare
Thank you for your suggestions. @fhahn @kuhar The code has been modified according to the review comments. The background for adding this func is because I am trying to optimize MLIR's data flow analysis. MLIR's dense data flow analysis attaches a lattice to all program points, while many of these program points do not actually change the lattice before and after, thus causing significant space waste. Additionally, it also causes a large amount of compilation time waste when iterating to a fixed point in the data flow. I hope to use an equivalence class to group the lattices that can be analyzed as definitely identical into one class, to reduce the overhead of iteration and lattice storage. In MLIR's data flow analysis, to facilitate the removal of operations by the pattern rewriter, a function called eraseState has been added to remove some lattice anchors from the solver. Correspondingly, I need the equivalence class to have the functionality to remove elements. |
|
ping |
|
Thank you for your suggestion, the code has been modified according to your comments. @matthias-springer |
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
I see, it would be great if you also could share the follow-up PR using the functionality
Co-authored-by: Florian Hahn <[email protected]>
Will do. |
cxy-1993
force-pushed
the
erase-equivalent
branch
from
b3c0853 to
d16663b
Compare
Thank you for your comment, code updated based on your comments. @kuhar |
Introduce 'erase(const ElemTy &V)' member function to allow the deletion of a certain value from EquivClasses. This is essential for certain scenarios that require modifying the contents of EquivClasses. --------- Co-authored-by: Florian Hahn <[email protected]>
5 participants
Introduce 'erase(const ElemTy &V)' member function to allow the deletion of a certain value from EquivClasses. This is essential for certain scenarios that require modifying the contents of EquivClasses.