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

Some nits, and potentially a bug in formula.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15018
+                                        const SCEV *&LHS, const SCEV *&RHS) {
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        SCTy = MinMax->getSCEVType();
----------------
caojoshua wrote:
> SequentialMinMax SCEVs can be applied here as well.
I'd prefer it to be a separate patch, if it's legal at all. Need to check carefully how poison flows in these formulae.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15021
+        LHS = MinMax->getOperand(0);
+        RHS = MinMax->getOperand(1);
+        if (isa<SCEVConstant>(LHS))
----------------
MinMax can have more than 2 operands. Check that it is exactly 2 of them?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15030
+    // Return a new SCEV that modifies \p Expr to the closest number divides by
+    // \p Divisor and greater than Expr.
+    auto GetNextSCEVDividesByDivisor = [&](const SCEV *Expr,
----------------
greater or equal


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15033
+                                           const SCEV *Divisor) {
+      if (!isKnownNonNegative(Expr) || !isKnownPositive(Divisor))
+        return Expr;
----------------
If I'm reading this correctly, this is a guarantee of no-overflow for computations you are doing. Maybe add this comment explicitly?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15038
+        // return the SCEV: Expr + Divisor - Expr % Divisor
+        return getAddExpr(getMinusSCEV(Divisor, Rem), Expr);
+      }
----------------
caojoshua wrote:
> Have not thought about it too deeply yet, but I'm concerned that we may need to take a look at when this is legal given Expr's NoWrapFlags. Same for the equivalent getMinusSCEV below
This computation is inconsistent. If `Rem` is constant `0`, you'll return `Expr`. But if `Rem` is effectively zero, but not a constant (e.g. some complex expression which is always zero), you return `Expr + Divisor`. Bug?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15044
+    // Return a new SCEV that modifies \p Expr to the closest number divides by
+    // \p Divisor and less than Expr.
+    auto GetPreviousSCEVDividesByDivisor = [&](const SCEV *Expr,
----------------
less or equal


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15052
+        // return the SCEV: Expr - Expr % Divisor
+        return getMinusSCEV(Expr, Rem);
+      }
----------------
I think you can safely drop check for `Rem->isZero()` here as it will be trivially simplified away in `getMinusSCEV`


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15060
+    // Divisor.
+    std::function<const SCEV *(const SCEV *, const SCEV *)>
+        ApplyDivisibiltyOnMinMaxExpr = [&](const SCEV *MinMaxExpr,
----------------
auto


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15123
+    // DividesBy.
+    std::function<bool(const SCEV *, const SCEV *&)> HasDivisibiltyInfo =
+        [&](const SCEV *Expr, const SCEV *&DividesBy) {
----------------
auto


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


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15146
+    // Return true if Expr known to divide by \p DividesBy.
+    std::function<bool(const SCEV *, const SCEV *&)> IsKnownToDivideBy =
+        [&](const SCEV *Expr, const SCEV *DividesBy) {
----------------
auto


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

https://reviews.llvm.org/D141850