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

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added a comment.

The improved trip multiples from the test results look good. Ordering in applyLoopGuards is an issue. However, I think we can simplify this down a bit. What if we always applied min/max first, before we apply divisibility guards? For example, given:

  __builtin_assume(TC % 8 == 0);
  __builtin_assume(TC > 0);
  __builtin_assume(TC < 100);



1. apply max: `umax(1, TC)`
2. apply min: `umin(100, umax(1, TC))`
3. apply divisibility info: `8 * (umin(100, umax(1, TC))) / 8`

This makes divisibility info obvious. And traversing the SCEV, we can still see `TC > 0` and `TC < 100`. I believe that if we always apply max/min first, we will never lose this info. This approach seems much simpler and easier to understand.

FYI. I haven't been around that long, and this change is non-trivial enough where I prefer a longstanding developer to give the final approval. I will still be around to give my thoughts.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15018
+                                        const SCEV *&LHS, const SCEV *&RHS) {
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        SCTy = MinMax->getSCEVType();
----------------
SequentialMinMax SCEVs can be applied here as well.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15019
+      if (auto MinMax = dyn_cast<SCEVMinMaxExpr>(Expr)) {
+        SCTy = MinMax->getSCEVType();
+        LHS = MinMax->getOperand(0);
----------------
unused var


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15023
+        if (isa<SCEVConstant>(LHS))
+          std::swap(LHS, RHS);
+        return isa<SCEVConstant>(RHS);
----------------
Constant should always be on the left side. Lets not change that.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15038
+        // return the SCEV: Expr + Divisor - Expr % Divisor
+        return getAddExpr(getMinusSCEV(Divisor, Rem), Expr);
+      }
----------------
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


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15129
+            if (isa<SCEVConstant>(MulLHS))
+              std::swap(MulLHS, MulRHS);
+            if (auto Div = dyn_cast<SCEVUDivExpr>(MulLHS)) {
----------------
Shouldn't swap the constant. Assume constant is on left.


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

https://reviews.llvm.org/D141850