Skip to content

Commit eb806f1

Browse files
[ADT] Fix a bug in EquivalenceClasses::erase
This patch fixes a bug in EquivalenceClasses::erase, where we lose a leader bit in a certain scenario. Here is some background. In EquivalenceClasses, each equivalence class is maintained as a singly linked list over its members. When we join two classes, we concatenate the two singly linked lists. To support path compression, each member points to the leader (through lazy updates). This is implemented with the two members: class ECValue { mutable const ECValue *Leader, *Next; : }; Each member stores its leader in Leader and its sibling in Next. Now, the leader uses the Leader field to to point the last element of the singly linked list to accommodate the list concatenation. We use the LSB of the Next field to indicate whether a given member is a leader or not. Now, imagine we have an equivalence class: Elem 1 -> Elem 2 -> nullptr Leader and wish to remove Elem 2. We would like to end up with: Elem 1 -> nullptr Leader but we mistakenly drop the leader bit when we update the Next field of Elem 1 with: Pre->Next = nullptr; This makes Elem 1 the end of the singly linked list, as intended, but mistakenly clears its leader bit stored in the LSB of Next, so we end up with an equivalence class with no leader. This patch fixes the problem by preserving the leader bit: Pre->Next = reinterpret_cast<const ECValue *>( static_cast<intptr_t>(Pre->isLeader())); The unit test closely follows the scenario above.
1 parent cac0635 commit eb806f1

File tree

2 files changed

+25
-1
lines changed

2 files changed

+25
-1
lines changed

llvm/include/llvm/ADT/EquivalenceClasses.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,8 @@ template <class ElemTy> class EquivalenceClasses {
258258
// If the current element is the last element(not leader), set the
259259
// successor of the current element's predecessor to null, and set
260260
// the 'Leader' field of the class leader to the predecessor element.
261-
Pre->Next = nullptr;
261+
Pre->Next = reinterpret_cast<const ECValue *>(
262+
static_cast<intptr_t>(Pre->isLeader()));
262263
Leader->Leader = Pre;
263264
} else {
264265
// If the current element is in the middle of class, then simply

llvm/unittests/ADT/EquivalenceClassesTest.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,29 @@ TEST(EquivalenceClassesTest, SimpleErase4) {
108108
EXPECT_FALSE(EqClasses.erase(1));
109109
}
110110

111+
TEST(EquivalenceClassesTest, EraseKeepsLeaderBit) {
112+
EquivalenceClasses<int> EC;
113+
114+
// Create a set {1, 2} where 1 is the leader.
115+
EC.unionSets(1, 2);
116+
117+
// Verify initial state
118+
EXPECT_EQ(EC.getLeaderValue(2), 1);
119+
120+
// Erase 2, the non-leader member.
121+
EXPECT_TRUE(EC.erase(2));
122+
123+
// Verify that we have exactly one equivalence class.
124+
ASSERT_NE(EC.begin(), EC.end());
125+
ASSERT_EQ(std::next(EC.begin()), EC.end());
126+
127+
// Verify that 1 is still a leader after erasing 2.
128+
const auto *Elem = *EC.begin();
129+
ASSERT_NE(Elem, nullptr);
130+
EXPECT_EQ(Elem->getData(), 1);
131+
EXPECT_TRUE(Elem->isLeader()) << "The leader bit was lost!";
132+
}
133+
111134
TEST(EquivalenceClassesTest, TwoSets) {
112135
EquivalenceClasses<int> EqClasses;
113136
// Form sets of odd and even numbers, check that we split them into these

0 commit comments

Comments
 (0)