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



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4458
 
-    bool isDone() { return TraversalDone; }
+    bool isDone() const { return TraversalDone; }
   };
----------------
Why do you need this?

Even if you do, please just check it in separately without further review.


https://reviews.llvm.org/D33316