[PATCH] D30887: [ScalarEvolution] Predicate implication from operations

[Sanjoy Das via Phabricator via llvm-commits]

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

Hi Maxim,

First of all, great work!

Can you try to split this change up into the addition transform (which, as I said inline, probably makes sense to put in `ScalarEvolution::isKnownPredicateViaNoOverflow`) and the division transform?  If one depends one the other, then we can just put denote the dependencies on phabricator, and check them in in the right order.

Other than that I have a few comments inline and here:

This rule seems a bit restrictive:

`(A > X) && (B > X) && (A >= 0 || B >= 0) ===> (A + B) > X`.

Why not:

`(A >= 0 && B > X)  ==>  ((A +nsw B) > X`?

(and the symmetric variant with `A` and `B` swapped)?

I've not checked if the code matches the commit message, but in

`(A > X) && (-B <= X < 0) ===> (A / B > 0)`, what if `X` is `-10`, `A` is `-5` and `B` is `10`?  Won't `A / B` be `0` then?



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8501
+  if (Pred == ICmpInst::ICMP_SLT)
+    return isImpliedViaOperations(ICmpInst::ICMP_SGT, RHS, LHS, FoundRHS,
+                                  FoundLHS);
----------------
The usual way to do this is

```
if (Pred == ICmpInst::ICMP_SLT) {
  Pred = ICmpInst::ICMP_SGT;
  std::swap(LHS, RHS);
  std::swap(FoundLHS, FoundRHS);
}
```

Actually, you should not need to do this. `ScalarEvolution::isImpliedCond` should be handling this case for you.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8513
+  auto ExtendType = [&](const SCEV *S, Type *T) {
+    if (S->getType() == T)
+      return S;
----------------
There already is `getNoopOrSignExtend` that does the same thing.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8519
+  auto MaxType = [&](const SCEV *S1, const SCEV *S2) {
+    return S1->getType()->getIntegerBitWidth() >
+                   S2->getType()->getIntegerBitWidth()
----------------
Use `getTypeSizeInBits(S1->getType())` instead, that will do the right thing for pointer types.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8531
+
+  if (auto *Operation = dyn_cast<SCEVAddExpr>(LHS)) {
+    // Should not overflow.
----------------
Don't call it `Operation`, that name says nothing about where the value came from and what it represents.  For a small scope I would not mind `Operation`, but this scope is more than a few lines long.  I'd go with something more mnemonic, like `AddExprLHS` or `LHSAddExpr`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8532
+  if (auto *Operation = dyn_cast<SCEVAddExpr>(LHS)) {
+    // Should not overflow.
+    if (!Operation->hasNoSignedWrap())
----------------
Did you consider putting this logic in `ScalarEvolution::isKnownPredicateViaNoOverflow`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8535
+      return false;
+    auto *Op1 = Operation->getOperand(0);
+    auto *Op2 = Operation->getOperand(1);
----------------
Similarly, good names for these would be `LL` ("LHS of LHS"), and `RL` ("RHS of LHS").


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8540
+    //   (Op2 > RHS) => (LHS > RHS).
+    auto *ZeroRHS = getConstant(RHS->getType(), 0u, true);
+    auto *Op1Ext = ExtendType(Op1, OrigLHS->getType());
----------------
Use `getZero(RHS->getType())`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8551
+      // Now we know that at least one of the operands in non-negative. It
+      // implies that Op1 + Op2 >= min(Op1, Op2). If we prove that
+      // (Op1 > RHS) && (Op2 > RHS), then (Op1 + Op2) >= min(Op1, Op2) > RHS.
----------------
This is probably too expensive to put in here (at the very least, this is risky from a compile time perspective).  How far can we get just by looking at the ranges for each of the inputs (i.e. via calling `getRange` and doing some manipulations on the returned ranges)?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8559
+                                          FoundRHS));
+  } else if (auto *Operation = dyn_cast<SCEVUnknown>(LHS)) {
+    Value *Op1, *Op2;
----------------
Same comment w.r.t. naming -- let's call it something more specific than `Operation`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8565
+      // Rules for division.
+      auto *Num = getSCEV(Op1);
+      auto *Denum = getSCEV(Op2);
----------------
Let's avoid creating SCEV expressions here (via `getSCEV` or `getAddExpr`).  One problem is that it may be a compile time hit.  They can also cause use to compute overly conservative trip counts, since this call to `isImpliedViaOperations` may have itself been done during a trip count computation, and invoking `getSCEV` may try to (recursively) compute the trip count of the same loop again which would cache a conservative `SCEVCouldNotCompute` (to avoid recursing infinitely).


https://reviews.llvm.org/D30887