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

[Sanjoy Das via llvm-commits]

On Mon, Aug 17, 2015 at 1:15 PM, hfinkel at anl.gov <hfinkel at anl.gov> wrote:
> 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);
>   };

Right -- I'd just tack something inexpensive to this lambda, like
`IsKnownPredicateViaNoWrap` that knows things like `{0,+,1}<nsw>` is
`slt` `{1,+,1}<nsw>`, `{0,+,1}<nuw>` is `ult` `{1,+,1}<nuw>` etc.

>> 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).

If you really wanted to do this, then you could get around the
infinite recursion using like PendingLoopPredicates.

-- Sanjoy