[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:
> > 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).
> Yes, I got it. I'll try to get a statistic on my hard case to check actual sizes of both caches (for an entire `CompareSCEVComplexity` call tree). It seems if they will be quite big and will have almost same size it will be better to combine caches of pairs `(Value *, Value *)` separately.
> 
> What do you think about improving caches further to cache all already estimated values, not only zeros?
> What do you think about improving caches further to cache all already estimated values, not only zeros?

IIUC that won't help as much since (by design) only the last leaf query and the "path" from the last leaf query to the root query in `CompareXXXComplexity` returns a non-zero value, so it sound less "bang for the buck" to me.  The big gains are by speeding up expressions that look "almost" equal which you've already addressed in this patch.

However, if you want to do the work to cache non-zero complexity differences then I won't stop you. :)


https://reviews.llvm.org/D26389