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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Comments inline.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:2183
+/// of this Phi is dominated by the loop L.
+static bool isComplexRecBelowLoop(const SCEV *S, const Loop *L,
+                                  DominatorTree &DT, LoopInfo &LI) {
----------------
I'd not call this complex rec, since I don't think this is specific to PHI nodes (see below).  Instead, I'd just call it `SCEVDominatesLoop` or something like that.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:2184
+static bool isComplexRecBelowLoop(const SCEV *S, const Loop *L,
+                                  DominatorTree &DT, LoopInfo &LI) {
+  switch (static_cast<SCEVTypes>(S->getSCEVType())) {
----------------
Can you use `SCEVTraversal` here? That'll also prevent exponential complexity.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:2210
+      auto *SU = dyn_cast<SCEVUnknown>(S);
+      if (auto *Phi = dyn_cast<PHINode>(SU->getValue()))
+        if (auto *PL = LI.getLoopFor(Phi->getParent()))
----------------
I'm not sure why this can't happen with non-PHIs.  IMO the condition should be "the expression the SCEVUnknown corresponds to must dominate L->getHeader()".


https://reviews.llvm.org/D33316