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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Hi Daniil,

In https://reviews.llvm.org/D26389#590731, @dfukalov wrote:

> I've reduced testcase to 400 lines. Then I tried to use cache as you suggested and it fixes the issue indeed. I'm going to re-test and up[date patch.


Great!

As I said before, if the test case is repetitive then it is probably better to generate the IR in memory using IRBuilder and put the test case in `unittests/`.  Otherwise a 400 line file does not sound that bad.

> But I would add recursion depth control since there is no guarantee the cache will help in all possible cases. For example, during test case reduction I had input variants of input that caused hangs in CompareSCEVComplexity called from other (not SLSR) passes.

I think a recursion depth as a stopgap measure is fine.  For now I'd add a max depth as a `cl::opt` and use the same depth in both `CompareValueComplexity` and `CompareSCEVComplexity` (that is, don't "forget" the depth when `CompareSCEVComplexity` calls into `CompareValueComplexity`).

I'd make this max depth fairly high by default (perhaps 64 or something) since, as I understand it, the caching should take care of avoiding any exponential behavior, and if the we're spending lots of time in `CompareSCEVComplexity` then the SCEV expression itself is very complex.  In this (latter) case it is defensible for `CompareSCEVComplexity` to take time (it simply has more work to do), but only up to a point.

> What do you think? If you agree with recursion depth, should it be new parameter (as in my first implementation) or a hard-coded value as for CompareValueComplexity?




https://reviews.llvm.org/D26389