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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev 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())) {
----------------
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. 


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4458
 
-    bool isDone() { return TraversalDone; }
+    bool isDone() const { return TraversalDone; }
   };
----------------
sanjoy wrote:
> Why do you need this?
> 
> Even if you do, please just check it in separately without further review.
Yeah, this wasn't intended to be here. Will be removed.


https://reviews.llvm.org/D33316