[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 Max,

I have some more comments.  This time I was not able to be as thorough as I wanted to be (got busy with other things during the day, and now it is bed time :) ), but hopefully whatever comments I have will let you make some progress.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8532
+  if (auto *Operation = dyn_cast<SCEVAddExpr>(LHS)) {
+    // Should not overflow.
+    if (!Operation->hasNoSignedWrap())
----------------
mkazantsev wrote:
> sanjoy wrote:
> > Did you consider putting this logic in `ScalarEvolution::isKnownPredicateViaNoOverflow`?
> Actually isKnownPredicateViaNoOverflow already does a particular case of it, which is "X + C > X if C > 0". The point here is that I need the context of FoundLHS and FounrRHS for further proofs. For example, I want to prove thing like "1 + n / 2 > 1 if n > 1". The rule which is used in isKnownPredicateViaNoOverflow does not work here, because n/2 does not match with 0 or 1. It also is unable to prove that n/2 > 0, because for this we need the proof via operation that uses context.
> 
> For the same reason I don't want to split this into two patches, because the only real benefit of the rule for addition is that it can recursively invoke the rule for division with the same context, and vice versa. Without the division rule, this addition work simply duplicates the logic of "isKnownPredicateViaNoOverflow".
> 
> I have removed "isKnownPredicate" invocation from here to avoid potential recursion, replacing it with more light-weight check. But we cannot move it out of here due to this context restriction.
I may have missed it, but I did not see (in the current version of the patch) where the division case calls into the addition case.

The addition case calling into the division case is fine, but I want to avoid "arbitrary recursion" by recursively calling into `isImpliedCondOperandsHelper`.  Can you structure the code in a way that that doesn't happen?  Maybe extract out the division case into a separate function that you directly call from here?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8565
+      // Rules for division.
+      auto *Num = getSCEV(Op1);
+      auto *Denum = getSCEV(Op2);
----------------
mkazantsev wrote:
> sanjoy wrote:
> > 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).
> I have removed the call of "isKnownPredicate" that might request recursive recalculation of the same trip count; now we use more light-weight check that used to be "IsKnownPredicateFull" lambda. So now this problem should have gone.
> 
> As for the compile time issue, what is the alternative to using SCEV for Num/Denum/FoundRHS+ 1?
One possibility would be to put the actual core of the logic in `llvm::isImpliedCondition` (which is in ValueTracking), and then try to call into that helper from there.

That is, say the antecedent is `(sext i16 %t to i32) s< i32 44` and the consequent is `%s != 400`.  We could then ask ValueTracking `llvm::isImpliedCondition(i16 %t s< i16 44, %s != 400)` [0] and return whatever ValueTracking told us.

We will probably need to generalize the interface of ValueTracking's `llvm::isImpliedCondition` a bit though, but that should be fine.

[0] Using the fact that `(sext(A) s< sext(B)) == A s< B`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8527
+    auto *Zero = getZero(RHS->getType());
+    auto *LLExt = getNoopOrSignExtend(LL, OrigLHS->getType());
+    auto *LRExt = getNoopOrSignExtend(LR, OrigLHS->getType());
----------------
Can we get what we want here without sign extension?  As I've said below, sign extension can be expensive.

In fact, it would be surprising if we see `LHS` is not the same as `OrigLHS` since that would mean a `sext (%a + %b)<nsw>` did not get transformed to `(sext %a + sext %b)<nsw>` as per the rule in `ScalarEvolution::getSignExtendExpr`.  That situation is possible, but should be rare.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8530
+
+    auto AdditionRule = [&](const SCEV *S1, const SCEV *S2) {
+      if (isKnownNonNegative(S1) ||
----------------
Add a one liner above this stating what this function is checking for.  If you can give it a better name then that would be even better.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8549
+    if (match(LHSUnknownExpr->getValue(), m_SDiv(m_Value(LL), m_Value(LR)))) {
+      auto MaxType = [&](const SCEV *S1, const SCEV *S2) {
+        auto Size1 = getTypeSizeInBits(S1->getType());
----------------
This seems general enough to me that we should put this on ScalarEvolution itself, as `Type *ScalarEvolution::getWiderType(Type *, Type *)`.  It also makes `isImpliedViaOperations` less cluttered.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8558
+      auto *Ty1 = MaxType(Num, FoundLHS);
+      auto *NumExt = getNoopOrSignExtend(Num, Ty1);
+      auto *FoundLHSExt = getNoopOrSignExtend(FoundLHS, Ty1);
----------------
Sign extending has the same problem as calling `getSCEV` (as you can probably tell from looking at `ScalarEvolution::getSignExtendExpr`, it can do a lot of work in the worst case).  It isn't terrible because SCEV will cache the result in most cases once it has computed it, but we should try very hard to not call it so deep in the stack.


https://reviews.llvm.org/D30887