[PATCH] D89381: [SCEV] Re-enable "Use nw flag and symbolic iteration count to sharpen ranges of AddRecs", attempt 3

Wed Oct 21 21:52:54 PDT 2020

mkazantsev added a comment.

Thoughts on https://llvm-compile-time-tracker.com/compare.php?from=4b7dafd9046f0ceaadacaafe0ea4a1fb00cf70a5&to=c713c1ef75612a3dae1ef4b99f766e0c7784381f&stat=instructions:

- Current implementation only creates one SCEV that isn't a constant, which is `End`. Implication methods now are as simple as they could be, and they do not create new SCEVs either.
- The new logic is definitely not free compile-time-wise, but out of 11 benchmark on that list it's greater than 1% only on two of them in O3 <https://reviews.llvm.org/owners/package/3/> mode.
- It would be interesting to know if it's the same with O1 <https://reviews.llvm.org/owners/package/1/> and O2 <https://reviews.llvm.org/owners/package/2/>; if problem only shows up on O3 <https://reviews.llvm.org/owners/package/3/> then it's much less important.
- The code in the mentioned file `partSalignmm.c` contains dozens of nested/sibling loops. It's exactly the case where SCEV can be expensive in general, and it doesn't look like a typical code seen everywhere.

In summary, we are facing the compile time regression on some corner case, which is definitely a problem we want to address, but I still can't see how is this a reason to keep the patch away. I'm pretty sure that against any transform or analysis it's possible to find a degenerate case where it will take huge amount of time.

I agree that it would be nice if we could avoid creating non-constant SCEV at all and only use constant ranges, but the motivating example shows that it's completely impossible for unsigned ranges & negative steps.

I think to speed this up, we might want to add more caching (maybe cache symbolic BE count), but it looks like something out of scope of this patch, where everything that could be cut was cut already.

Thoughts?

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

https://reviews.llvm.org/D89381