[PATCH] D88087: [SCEV] Limited support for unsigned preds in isImpliedViaOperations

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10158
+      const SCEV *MinusOne = getNegativeSCEV(getOne(LHS->getType()));
+      if (isImpliedCondOperands(ICmpInst::ICMP_SGT, LHS, MinusOne, FoundLHS,
+                                FoundRHS) &&
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > reames wrote:
> > > I'm confused by this check, it seems overly complicated given the comment?  Why is this simply not:
> > > if (isKnownNonNegative(LHS) && isKnownNonNegative(RHS) &&
> > >     isKnownNonNegative(FoundLHS) && isKnownNonNegative(FoundRHS)) {
> > >    Pred = ICmpInst::getSignedPredicate(Pred);
> > > }
> > Because non-negativity of LHS and RHS is not known. 
> We need an extra fact `SignedPred FoundLHS FoundRHS` to prove it. And we only have this fact if we know `Pred FoundLHS FoundRHS && isKnownNonNegative(FoundLHS) && isKnownNonNegative(FoundRHS)`.
If FoundLHS and FoundRHS are both non-negative, then all unsigned predicates are equivalent to their signed variants.  There's no separate fact needed for that.  Same for LHS and RHS.

Max, it may help if we talked offline here.  I can't tell if you're saying my argument is wrong, or that the example your trying to optimize doesn't have sufficient information to establish the non-negative facts.  Please either talk to me offline, or be very specific in your next response.


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

https://reviews.llvm.org/D88087