[PATCH] D12073: Make ScalarEvolution::isKnownPredicate a little smarter

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

To look at this a bit more (i.e. why does this actually make the test case pass), we have this:

SimplifyIndvar::eliminateIVComparison has this code:

  } else if (SE->isKnownPredicate(ICmpInst::getInversePredicate(Pred), S, X)) {
    ICmp->replaceAllUsesWith(ConstantInt::getFalse(ICmp->getContext()));
    DeadInsts.emplace_back(ICmp);
    DEBUG(dbgs() << "INDVARS: Eliminated comparison: " << *ICmp << '\n');

Pred is CmpInst::ICMP_SGT, so the inverse predicate is ICMP_SLE.

And is calling it with:

  S = {0,+,1}<nuw><nsw><%for.body> and X = %n

ScalarEvolution::isKnownPredicate first calls SimplifyICmpOperands, which rewrites LHS and RHS to be:

  LHS = {-1,+,1}<nsw><%for.body>, RHS = %n, and sets Pred = CmpInst::ICMP_SLT

ScalarEvolution::isKnownPredicate then ends of here:

  if (LAR) {
    const Loop *L = LAR->getLoop();
    if (isLoopEntryGuardedByCond(L, Pred, LAR->getStart(), RHS) &&
        isLoopBackedgeGuardedByCond(L, Pred, LAR->getPostIncExpr(*this), RHS)) {
      if (!RAR) return true;

(where RAR is nullptr here because `%n` is not an AddRec).

At this point, we end up in ScalarEvolution::isLoopBackedgeGuardedByCond where
we have:

  Pred = CmpInst::ICMP_SLT, LHS = {0,+,1}<nuw><nsw><%for.body> and RHS = %n

ScalarEvolution::isLoopBackedgeGuardedByCond finds the loop backedge predicate:

  br i1 %cmp, label %for.body, label %for.cond.for.cond.cleanup_crit_edge

being fed by:

  %cmp = icmp slt i32 %inc, %n

and calls ScalarEvolution::isImpliedCond, and that calls ScalarEvolution::isImpliedCondOperands with:

  FoundLHS = {1,+,1}<nuw><nsw><%for.body>, FoundRHS = %n

where we still have:

  LHS = {0,+,1}<nuw><nsw><%for.body>, RHS = %n

that calls ScalarEvolution::isImpliedCondOperandsHelper, and that ends up here:

  case ICmpInst::ICMP_SLT:
  case ICmpInst::ICMP_SLE:
    if (IsKnownPredicateFull(ICmpInst::ICMP_SLE, LHS, FoundLHS) &&
        IsKnownPredicateFull(ICmpInst::ICMP_SGE, RHS, FoundRHS))
      return true;

IsKnownPredicateFull is this lambda function:

  auto IsKnownPredicateFull =
    [this](ICmpInst::Predicate Pred, const SCEV *LHS, const SCEV *RHS) {
      return isKnownPredicateWithRanges(Pred, LHS, RHS) ||
             IsKnownPredicateViaMinOrMax(*this, Pred, LHS, RHS);
  };

and it is this call to ScalarEvolution::isKnownPredicateWithRanges, with:

  LHS = {0,+,1}<nuw><nsw><%for.body> and RHS = {1,+,1}<nuw><nsw><%for.body> that the code added in this patch triggers to provide an answer we could not previously provide.

And so, yes, you're right, putting this logic in isImpliedCondOperandsHelper seems like a practical place.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6892
@@ -6891,1 +6891,3 @@
       return false;
+
+    const SCEV *Diff = getMinusSCEV(LHS, RHS);
----------------
hfinkel wrote:
> sanjoy wrote:
> > I'm not sure this is sound w.r.t. overflow.  For instance, if `LHS` = `i8 127` and `RHS` = `i8 -1` then `!(LHS < RHS)` but `LHS - RHS = i8 128 < 0`
> This is precisely what was worrying me w.r.t. to correctness, and given the output, it seems this is exactly what happens. Sign-extending first would fix that, right?
As it turns out, sign extending works, but can't be used here. The problem is that, under certain circumstances, getSignExtendExpr calls isLoopBackedgeGuardedByCond, and that calls isKnownPredicateWithRanges (yielding an infinite recursion).



Repository:
  rL LLVM

http://reviews.llvm.org/D12073