[ADT] Fix IntEqClasses::join to return the leader in all cases.

[jcranmer-intel]

Summary

The method is specified to return the leader of the result of a join. If
the two members were already in an equivalence class, there is a chance
that it might fail to return the leader. For example, if the array looks
like this:

EC[0] = 0
EC[1] = 0
EC[2] = 1
EC[3] = 1

Then join(2, 3) would have returned 1, which is not the same as the
actual leader 0.

[MatzeB]

Nice catch!

llvm/lib/Support/IntEqClasses.cpp

36–52

Not sure how this would affect worst-case complexity given findLeader does not do any path compression. How about this instead?

[jcranmer-intel]

llvm/lib/Support/IntEqClasses.cpp

36–52

The changes I made wouldn't make the worst-case complexity any worse; it seems to me that it would work better to have findLeader do path compression instead (I further note that there appear to be no callers of IntEqClasses::findLeader in LLVM at present, so there's definitely no compile time regression here).

[jcranmer-intel]

ping?

[MatzeB]

llvm/lib/Support/IntEqClasses.cpp

36–52

The changes I made wouldn't make the worst-case complexity any worse

They obviously won't affect the case when we only call join on members of different sets. However without the changes I proposed (or adding path compression to findLeader) it's not obvious to me that we won't cannot hit bad complexity for the cases of calling join with on members of the same set (this is only a gut feeling without proof, but you're not presenting an argument against that either).

Anyway any reason not to go with my proposed changes? Anyway adding path-compression to findLeader should also work... Either way you should upload one variant :)

[jcranmer-intel]

Add support for path compression to IntEqClasses::findLeader.

llvm/lib/Support/IntEqClasses.cpp

 //===-- llvm/ADT/IntEqClasses.cpp - Equivalence Classes of Integers -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
void IntEqClasses::grow(unsigned N) {
   while (EC.size() < N)
     EC.push_back(EC.size());
 }
 
 unsigned IntEqClasses::join(unsigned a, unsigned b) {
   assert(NumClasses == 0 && "join() called after compress().");
   unsigned eca = EC[a];
   unsigned ecb = EC[b];
   // Update pointers while searching for the leaders, compressing the paths
   // incrementally. The larger leader will eventually be updated, joining the
   // classes.
   while (eca != ecb)
     if (eca < ecb) {
       EC[b] = eca;
       b = ecb;
       ecb = EC[b];
     } else {
       EC[a] = ecb;
       a = eca;
       eca = EC[a];
     }
 
+  // Make sure that eca is actually the leader of the equivalence class (this
+  // can happen if a and b were already in the same equivalence class).
+  eca = findLeader(eca);
   return eca;
 }
 
 unsigned IntEqClasses::findLeader(unsigned a) const {
   assert(NumClasses == 0 && "findLeader() called after compress().");
   while (a != EC[a])
     a = EC[a];
   return a;

llvm/unittests/ADT/IntEqClassesTest.cpp

 //===---- ADT/IntEqClassesTest.cpp - IntEqClasses unit tests ----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
TEST(IntEqClasses, Simple) {
   EXPECT_EQ(4u, ec.findLeader(4));
   EXPECT_EQ(4u, ec.findLeader(5));
   EXPECT_EQ(6u, ec.findLeader(6));
   EXPECT_EQ(6u, ec.findLeader(7));
   EXPECT_EQ(4u, ec.findLeader(8));
   EXPECT_EQ(0u, ec.findLeader(9));
 }
 
+TEST(IntEqClasses, JoinReturnsLeader) {
+  IntEqClasses ec(10);
+  EXPECT_EQ(1u, ec.join(1, 2));
+  EXPECT_EQ(1u, ec.join(1, 3));
+  EXPECT_EQ(0u, ec.join(0, 1));
+  EXPECT_EQ(0u, ec.findLeader(1));
+  // At this point, EC[2] = EC[3] = 1, but EC[1] = 0. Be sure that trying to
+  // join EC[2] and EC[3] returns the correct leader, 0, and not their common
+  // node 1.
+  EXPECT_EQ(0u, ec.join(2, 3));
+  EXPECT_EQ(0u, ec.findLeader(2));
+}
+
 } // end anonymous namespace