[PATCH] D33316: [SCEV] Do not fold expressions with SCEVUnknown Phis into AddRecExpr's

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:2197
+    bool checkSCEVUnknown(const SCEVUnknown *SU) {
+      if (auto *I = dyn_cast<Instruction>(SU->getValue())) {
+        if (DT.dominates(L->getHeader(), I->getParent())) {
----------------
mkazantsev wrote:
> sanjoy wrote:
> > I believe this should be more general, and we should disallow forming `{%x,+,%y}<%loop>` unless `%x` and `%y` both dominate `%loop`[0].  Whether there is a PHI involved or not should not matter.
> > 
> > In particular, `{%x,+,%y}` is an expression that evaluates to `%x` on the 0th iteration of the loop.  This is meaningless unless `%x` dominates the loop header.
> > 
> > However, if you fix this for good, you //may// see some fall out of LSR, but we should just go ahead and fix those.
> > 
> > [0]: or, if they're complex SCEV expressions, all of the contained SCEVUnknown and SCEVAddRec expressions must dominate `%loop`.
> > 
> Consider two situations:
> 
> Case 1:
>   for (...) {
>     %phi = {a,+,b}
>   }
>   %x = ...
>   %y = %phi + %x
> 
> Case 2:
>   %x = ...
>   for (...) {
>     %phi = {a,+,b}
>   }
> 
>   %y = %phi + %x
> 
> If %x doesn't depend on the loop in any way, I don't see a reason why SCEV should not evaluate %y equally in both cases. In case 2, it is absolutely OK to imagine a recurrence {a+x,+,b} (it could be explicitly calculated in the loop by creating a Phi), but in case 1 such transormation is also valid because SCEV should be place-independent, and if nothing prohibits us from calculating %x before the loop (even if the actual instruction stands after it), we should be able to deal with it just like if it was calculated before the loop if it makes sense. Is it not correct?
> 
> The only problem SCEVUnknown creates is that it can be actually a varying value in some loop which does not dominate L. In this case our logic of picking the bottom-most loop in getAddExpr and getMulExpr becomes incorrect. We are just picking the wrong loop, because we can only pick a loop of AddRecExpr.
> 
> If there are no loops with Phis between our SCEVUnknown and L, all varying values that might be used by the SCEVUnknown actually dominate the loop L (and this is actually what we want in [0]). It means that our picking of the loop in AddExpr is correct. We could just prohibit all SCEVUnknown below L, but I think this would be too strong reduction of the scope. 
I don't think the two situations are the same.

Firstly, I agree that in case 2, `%y` should be `{%x+a,+,b}`.

In case 1, however, I don't think the same should hold.  The definition of a `{a,+,b}` is that it is equal to `a` on the 0th iteration and that makes little sense if `a` does not dominate the loop.  For instance, if we allow such SCEV expressions, we won't always be able to compute a "value at exit" (like we do in `rewriteLoopExitValues`) that is available on a loop exit.

However, repeating what I said before, I suspect we may have to compromise on this due to LSR (i.e. LSR may be relying on us also folding case 1), but we should try and see.



https://reviews.llvm.org/D33316