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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.

Mostly looks good, but I think I found a bug in `IsMinMaxSCEVWithConstant `. Also please rebase on top of your tests.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15031
+        RHS = MinMax->getOperand(1);
+        return isa<SCEVConstant>(RHS);
+      }
----------------
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.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15169
+            return true;
+          if (isa<SCEVMinMaxExpr>(Expr)) {
+            auto MinMax = cast<SCEVMinMaxExpr>(Expr);
----------------
`if (auto *MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) { ...`


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15083
+          auto IsMin =
+              isa<SCEVSMinExpr>(MinMaxExpr) || isa<SCEVUMinExpr>(MinMaxExpr);
+          auto DivisibleExpr =
----------------
alonkom wrote:
> mkazantsev wrote:
> > Do you really support `smin` here? Your code is only correct for unsigned values. It interprets all values as non-negative.
> I check if the operands are non-negative inside GetPreviousSCEV/GetNextSCEVD
Ah ok, then it makes sense. Can we assert on that here?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15191
+    case CmpInst::ICMP_SLT: {
+      auto ModifiedRHS = getMinusSCEV(RHS, getOne(RHS->getType()));
+      ModifiedRHS =
----------------
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. :)



================
Comment at: llvm/test/Analysis/ScalarEvolution/trip-multiple-guard-info.ll:397
 
+define void @test_trip_multiple_4_upper_lower_bounds(i32 %num) {
+; CHECK-LABEL: 'test_trip_multiple_4_upper_lower_bounds'
----------------
alonkom wrote:
> mkazantsev wrote:
> > Please precommit tests as is, I want to see what exactly this patch changes.
> https://reviews.llvm.org/D143337
Rebase patch on top of it?


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

https://reviews.llvm.org/D141850