[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/lib/Transforms/Scalar/IndVarSimplify.cpp:1425
+    return false;
+  auto *BI = dyn_cast<BranchInst>(ExitingBB->getTerminator());
+  if (!BI || BI->isUnconditional())
----------------
mkazantsev wrote:
> Looks like a great place to use pattern matching.
Its not.  :)  I need to mutate the condition of the ICmp below, and the pattern matching code is just more complicated than what is here.  I tried it; it's ugly.  :)


================
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:%.*]]
----------------
reames wrote:
> 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.  
I'd always planned to handle this in a follow up, but since you asked: D111896.

(We still have to work around the fact simplifyAndExtend never visits the icmp and avoid poisoning SCEV with sub-optimal results when we do.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111836