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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

Ok, thanks, let's give it a try.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15052
+      APInt Rem = ExprVal.urem(DivisorVal);
+      if (!Rem.isZero()) {
+        // return the SCEV: Expr + Divisor - Expr % Divisor
----------------
nit: `{}` not needed


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15064-15071
+      if (!isKnownNonNegative(Expr) || !isKnownPositive(Divisor))
+        return Expr;
+      auto *ConstExpr = dyn_cast<SCEVConstant>(Expr);
+      auto *ConstDivisor = dyn_cast<SCEVConstant>(Divisor);
+      if (!ConstExpr || !ConstDivisor)
+        return Expr;
+      APInt ExprVal = ConstExpr->getAPInt();
----------------
This code is copy-paste-ish (see lambda above), maybe factor out?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15078
+    // Apply divisibilty by \p Divisor on MinMaxExpr with constant values,
+    // recursively. This is done by aligning up/down the constant value to the
+    // Divisor.
----------------
If you see any problems with compile time from this patch, this is a potential place where things can be limited. Not sure if it's worth it in practice.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15091
+                 "Expected non-negative operand!");
+          auto DivisibleExpr =
+              IsMin ? GetPreviousSCEVDividesByDivisor(MinMaxLHS, Divisor)
----------------
nit: `auto*`


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15112
+          const SCEV *RewrittenLHS =
+              I != RewriteMap.end() ? I->second : LHSUnknown;
+          RewrittenLHS = ApplyDivisibiltyOnMinMaxExpr(RewrittenLHS, URemRHS);
----------------
Is it possible that the map contains both `LHSUnknown -> RewrittenLHS ` and `RewrittenLHS  -> SomeOtherLHS`? Should this be a loop?

I'm ok if it's a separate patch.


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

https://reviews.llvm.org/D141850