[PATCH] D137505: [SCEV] Cache ZExt SCEV expressions.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

One thing we can do in verifier is to check that `FoldCache` and `FoldCacheUser` are in sync, meaning that for each `{SCEV, FoldID}` in `FoldCacheUser` there is `{FoldId, SCEV}` in `FoldCache` and vice versa. Just in case if one of them will be erased without doing the 2nd part.



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:73
+
+class FoldID {
+  SmallVector<unsigned, 4> Bits;
----------------
It looks general enough to be moved to some utils, no? Just to think in the future.


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:79
+  void addInteger(unsigned I) { Bits.push_back(I); }
+  void addInteger(int I) { Bits.push_back(I); }
+
----------------
The fact that we have different API for signed and unsigned leads me to think that we might be able to distinguish between them, e.g. distinguish set with single `(int)1` from set with `(unsigned) 1`, but in fact we can't. What was the intention?

Maybe better remove signed version.


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:84
+    addInteger(unsigned(I >> 32));
+  }
+
----------------
What worries me is that how easy to get a collision here. Basically, `{0, 0, ~0, ~0, 0, 0}` will collide with `{0ULL, ~0ULL, 0ULL}`. In your use case it should really be fine, because we add a lot of pointers, but if it is to be done more universal, need to think on making it harder to cheat (mix in types into hash?).


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:97
+  unsigned computeHash() const {
+    unsigned Hash = Bits[0];
+    for (unsigned I = 1; I != Bits.size(); ++I)
----------------
`assert(!Bits.empty())`?
Otherwise, if we want to allow empty sets (dunno why, just for generality), maybe start with `Hash =Bits.size()` and then mix in all elements?


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:125
+  static unsigned getHashValue(const FoldID &Val) {
+    return Val.computeHash() & ~(1 << (8 * sizeof(unsigned) - 1));
+  }
----------------
is it just `& std::max<int>()`?  Not asking to change, just trying to understand what was the intention.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137505/new/

https://reviews.llvm.org/D137505