[PATCH] D26389: [SCEV] limit recursion depth of CompareSCEVComplexity

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:704
+
+  SmallSet<std::pair<const SCEV *, const SCEV *>, 8> EqCache;
   if (Ops.size() == 2) {
----------------
dfukalov wrote:
> sanjoy wrote:
> > IMO a slightly better pattern would have been to have `EqCache` be a `SmallSet` of `std::pair<PointerUnion<Value *, const SCEV *>, PointerUnion<Value *, const SCEV *>>` and to re-use the same cache in `CompareValueComplexity`.
> > 
> > If you agree, can you do that as a followup change?
> I'm not sure this will actually be a re-use: any correct `SCEV *` is not equal to any correct `Value *` (except nullptr values), so the combined cache will contain all of items from both current caches, without memory usage reducing. And if it so, any search in a combined cache will be the same or slower.
> E.g. there is an edge case: we can try to find actual value typed `std::pair<const SCEV*, const SCEV*>` in a set that contains `std::pair<Value*, Value*>` only. Do you agree?
We might win in some edge cases by since we'll persist the same value cache across multiple `SCEVUnknown` instances in the same SCEV tree.  That is:

```
  %x = ...
  %y = ...
  %x1 = add %x, 20
  %y1 = add %y, 20
```

If LHS has SCEVUnknowns for %x and %x1, and RHS has  SCEVUnknowns for %y and %y1, and then a combined cache will save us from recomputing `CompareValueComplexity(%x, %y)` twice.

But this is a minimal gain -- please don't block the commit on getting this right.


https://reviews.llvm.org/D26389