[PATCH] D141850: [SCEV] Preserve divisibility and min/max information in applyLoopGuards

[Alon Kom via Phabricator via llvm-commits]

alonkom marked 4 inline comments as done.
alonkom added a comment.

@mkazantsev Let me know if you have any other comments



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15031
+        RHS = MinMax->getOperand(1);
+        return isa<SCEVConstant>(RHS);
+      }
----------------
mkazantsev wrote:
> 1. If I remember correctly, SCEV operands are always sorted by kind. It means that constants always go first. `RHS` can't be a constant because then `LHS` is also a constant and the whole thing should be folded away. So it should be trivially `false`, and I guess this code part isn't covered by tests if it wasn't caught.
> 2. Usually matcher methods aren't supposed to change operands if they return `false`. In your case it doesn't matter, but generally better to follow the standard practice and do all checks before modifying the operands.
> 
  # Nice catch! It didn't hurt the divisibility info, but some min/max info was lost. I Fixed this and added a unit-test since the lit only checks the divisibility.

  # Done.







================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15191
+    case CmpInst::ICMP_SLT: {
+      auto ModifiedRHS = getMinusSCEV(RHS, getOne(RHS->getType()));
+      ModifiedRHS =
----------------
mkazantsev wrote:
> alonkom wrote:
> > mkazantsev wrote:
> > > How do you account for `RHS` being `SINT_MIN` and similar cases here?
> > I wonder how this worked before my patch. 
> > if we have assume (N < SINT_MIN)  then the following SCEV was generated:
> > smin(N, SINT_MIN - 1) which would overflow.
> > 
> > 
> Well, if there is a check somewhere that `RHS` is non-negative, then it makes sense. Otherwise it's possibly broken. :)
> 
Anyway, the change I've made only applies to non-negative values


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

https://reviews.llvm.org/D141850