[PATCH] D43507: [SCEV] Fix isKnownPredicate

[Sanjoy Das via Phabricator via llvm-commits]

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

This change definitely needs some tests.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8753
+          // We have two Loops which does not dominate each other, so bail out.
+          return nullptr;
+      } else
----------------
Can we assert that this is not the case (i.e. that we never have two loops in the expression unrelated by dominance)?  If yes, perhaps we can do:

```
#ifdef NDEBUG
for (x, y) all pairs of loops in LoopsUsed:
  assert x dom y or y dom x
#endif
MDL = std::max(LoopsUsed, <compare function using dom tree>);
```



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8760
+
+  auto CheckThroughLoop = [](ScalarEvolution &SE, const Loop *L,
+                             ICmpInst::Predicate Pred, const SCEV *LHS,
----------------
This lambda is lifting a lot of weight -- I think it should be a separate function and documented explicitly.

How about extracting a separate function `SplitIntoInitAndPostInc` that returns a tuple of an init expression, post-inc expression and loop that you call from `isKnownPredicate`?  Please also give a simple concrete example of what's going on in the comment.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8772
+    // Compute post increment of LHS and RHS for loop L.
+    // We allow other loops then L to be in LHS/RHS.
+    auto *PostIncLHS = SCEVPostIncRewriter::rewrite(LHS, L, SE, true);
----------------
Should be "other loops than L"

Same applies above.


https://reviews.llvm.org/D43507