[PATCH] D40427: [ADT] Introduce Disjoint Set Union structure

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: include/llvm/ADT/DisjointSetUnion.h:42
+  // Check whether or not X and Y belong to one set after merges made so far.
+  bool isInSameSet(T X, T Y) {
+    return head(X) == head(Y);
----------------
You're copying `T` instances here -- why not take `const T &` instead?

(For `SCEV *` and `Value *` this does not matter, but e.g. people may eventually want to store `std::string` here).


================
Comment at: include/llvm/ADT/DisjointSetUnion.h:84
+      // recursively for all chain of parents.
+      return Parent[X] = head(It->second);
+    // Every vertex that doesn't have a parent is the head of its set.
----------------
Please avoid recursion here, unless you're certain this would be (say) less than 10 frames for all practical cases (in which case add an assert).


================
Comment at: include/llvm/ADT/DisjointSetUnion.h:103
+  // Stores immediate parents of vertices.
+  DenseMap<T , T> Parent;
+  // Memorizes calculated ranks of head vertices.
----------------
The pattern I've seen here is taking the map type as a template parameter instead of hardcoding it (with `DenseMap` as a default), so that folks can substitute `SmallDenseMap` etc.


================
Comment at: include/llvm/ADT/DisjointSetUnion.h:105
+  // Memorizes calculated ranks of head vertices.
+  DenseMap<T, int> Rank;
+};
----------------
How about just one `DenseMap` that maps `T` instances to a `std::pair<T, int>`?


https://reviews.llvm.org/D40427