[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.

I think supporting `smin/smax` is a bug, this code is only written for unsigned values.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15020
+                                        const SCEV *&LHS, const SCEV *&RHS) {
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        if (MinMax->getNumOperands() != 2)
----------------
nit: `auto *`


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15042
+        return Expr;
+      unsigned ExprVal = ConstExpr->getAPInt().getZExtValue();
+      unsigned DivisorVal = ConstDivisor->getAPInt().getZExtValue();
----------------
Do you check bit width anywhere? What if it doesn't fit into `unsigned`?

Better use APInt unless you have a reason not to.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15078
+                                           const SCEV *Divisor) {
+          const SCEV *MinMaxLHS, *MinMaxRHS;
+          SCEVTypes SCTy;
----------------
Init with nullptr, makes it easier to debug.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15083
+          auto IsMin =
+              isa<SCEVSMinExpr>(MinMaxExpr) || isa<SCEVUMinExpr>(MinMaxExpr);
+          auto DivisibleExpr =
----------------
Do you really support `smin` here? Your code is only correct for unsigned values. It interprets all values as non-negative.


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

https://reviews.llvm.org/D141850