[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) {
----------------
sanjoy wrote:
> 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.
So, for instance, persisting a `(Value *, Value *)` set for the entirety of `CompareSCEVComplexity` will also do the same thing (and will probably be mildly cleaner).


https://reviews.llvm.org/D26389