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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri 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) &&
----------------
reames wrote:
> 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.
> 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.

I agree. The actual rules are: (for any signed/unsigned pred, they just have to be identical)
https://rise4fun.com/Alive/P9Il


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

https://reviews.llvm.org/D88087