[PATCH] D51014: [SCEV] Compare SCEVs by a complexity rank before lexic-al comparison

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.

If these are two patches, please put them on review as 2 different patches and set dependency between them. You can always have them merged together, but will be easier to review. It is not clear which changes in tests are caused by which patch now. I believe that the part with revert can be merged unconditionally because it shows a functional problem, even if it costs us some compile time.

As for the algorithm, see comments inline. I think you should use `hash_combine` and collect some statistics to be sure that your assumptions about rare collisisons is right.



================
Comment at: include/llvm/Analysis/ScalarEvolution.h:92
+  // expressions behaves just like a hash.
+  const unsigned NestedComplexityRank : 25;
+  static constexpr unsigned NCMask = 0x1FFFFFF;
----------------
I just wonder if 32M different hashes is enough (maybe the statistics collection will help us understand).


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:146
+  unsigned getComplexityRank() const {
+    return (SCEVType + 1U) * getNestedComplexityRank();
+  }
----------------
You only compare ranks when the SCEVTypes of two SCEVs match (lines 636-639):
  // Primarily, sort the SCEVs by their getSCEVType().
  unsigned LType = LHS->getSCEVType(), RType = RHS->getSCEVType();
  if (LType != RType)
    return (int)LType - (int)RType;


There is no point to multiply by `(SCEVType + 1U)`.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:149
+  /// Return a rank used to compare complexities of same root type SCEVs.
+  unsigned getNestedComplexityRank() const { return NestedComplexityRank; }
+
----------------
Per comment above, how is that different from `getComplexityRank`?


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:28
 #include "llvm/Support/ErrorHandling.h"
+#include <numeric>
 #include <cassert>
----------------
Please arrange includes in lexicographic order.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:206
+                                       [](unsigned Acc, const SCEV *const Op) {
+                                         return Acc + Op->getComplexityRank();
+                                       })) {}
----------------
I guess what you want here is ` return Acc * X + Op->getComplexityRank();`, otherwise it's not even a polynomial hash.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:271
+               // For consistency with lexicographical order weigh LHS higher.
+               lhs->getComplexityRank() * scMulExpr + rhs->getComplexityRank()),
+          LHS(lhs), RHS(rhs) {}
----------------
Could you please explain why you multiply by `scMulExpr`? If you need some constant for polynomial hash computation then it's better to declare it separately. It's better be a prime number to have less collisions (I don't even know what number it is now).

Otherwise, you might be interested in using the function `hash_combine` which happens over and over LLVM. This will spare you from manual polynoms calculation.


I'd also suggest to make a separate method for hash computation to keep things encapsulated.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:698
+    if (LNCRank != RNCRank)
+      return (int)LNCRank - (int)RNCRank;
 
----------------
Could you please add the statistics to check how often do we have a hash collision? I.e. when hashes matched but further comparison returned something different from 0.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:767
 
+    const unsigned LNCRank = LC->getNestedComplexityRank();
+    const unsigned RNCRank = RC->getNestedComplexityRank();
----------------
Do we really need this? They have 1 argument, this check will be done if needed when we compare arguments, no?



Repository:
  rL LLVM

https://reviews.llvm.org/D51014