[PATCH] D111836: [indvars] Use fact loop must exit to canonicalize to unsigned conditions

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/test/Transforms/IndVarSimplify/finite-exit-comparisons.ll:304
 ; CHECK-NEXT:    [[ZEXT:%.*]] = zext i8 [[IV_NEXT]] to i16
-; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i16 [[ZEXT]], 254
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ugt i16 [[ZEXT]], 254
 ; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END:%.*]]
----------------
mkazantsev wrote:
> Why does it need mustprogress notion to do this at all? Zext is non-negative, 254 is non-negative, we can always do that regardless of mustprogress. We have this logic in `eliminateIVComparison`:
> 
> ```
>   } else if (ICmpInst::isSigned(OriginalPred) &&
>              SE->isKnownNonNegative(S) && SE->isKnownNonNegative(X)) {
>     // If we were unable to make anything above, all we can is to canonicalize
>     // the comparison hoping that it will open the doors for other
>     // optimizations. If we find out that we compare two non-negative values,
>     // we turn the instruction's predicate to its unsigned version. Note that
>     // we cannot rely on Pred here unless we check if we have swapped it.
>     assert(ICmp->getPredicate() == OriginalPred && "Predicate changed?");
>     LLVM_DEBUG(dbgs() << "INDVARS: Turn to unsigned comparison: " << *ICmp
>                       << '\n');
>     ICmp->setPredicate(ICmpInst::getUnsignedPredicate(OriginalPred));
> ```
> Why it didn't work?
See the comment in code about why this can't be done in simplifyAndExtend.  Short version: we never visit the icmp due to the presence of the zext, and if we do, we get overall worse results due to poisoning of SCEV caches. 

I tried that approach first, and gave up after getting tangled up in SCEV dance around partially constructed SCEVs and trip count logic.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111836/new/

https://reviews.llvm.org/D111836