[clang] [llvm] [EquivalenceClasses] Use SmallVector for deterministic iteration order. (PR #134075)

[Florian Hahn via llvm-commits]

================
@@ -138,6 +139,9 @@ class EquivalenceClasses {
   /// ECValues, it just keeps the key as part of the value.
   std::set<ECValue, ECValueComparator> TheMapping;
 
+  /// List of all members, used to provide a determinstic iteration order.
+  SmallVector<const ECValue *> Members;
----------------
fhahn wrote:

This is where things get a little tricky. `ECValue` contain pointers to other `ECValue`'s (the leader of the class and the next member of the class), so using something like `SetVector` as the backing storage won't work I think, as it might reallocate and move the objects.

One thing I tried is using a BumpAllocator for the elements in the class and a separate DenseMap instead of the std::set: https://github.com/llvm/llvm-project/commit/5c13864badfd7170c9c7c0a908e3cba8aa0f4fea

I think SetVector (or MapVector) would be slightly worse here, as it stores an ECValue in both a vector and a denseSet, and ECValue contains 3 pointers, so it would require quite a bit of extra memory .

The patch could use a `DenseSet`, it just requires a custom DenseMapInfo for ECValue to just hash/compare the data part. I can do that if desired and prepare the patch for review

Compile-time wise this looks neutral, but it would simplify things slightly https://llvm-compile-time-tracker.com/compare.php?from=b0c2ac67a88d3ef86987e2f82115ea0170675a17&to=5c13864badfd7170c9c7c0a908e3cba8aa0f4fea&stat=instructions:u

(the patch just has the unit tests for EQClass removed, because one of the keys doesn't have DenseMapInfo).



https://github.com/llvm/llvm-project/pull/134075