[PATCH] D48338: [SCEV] Improve zext(A /u B) and zext(A % B)

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1775
+
+  // zext(A / C) --> zext(A) / zext(C).
+  if (auto *Div = dyn_cast<SCEVUDivExpr>(Op)) {
----------------
Minor: why `C` instead of `B`?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12175
+
+bool ScalarEvolution::matchURem(const SCEV *Expr, const SCEV *&LHS,
+                                const SCEV *&RHS) {
----------------
For reference it would be nice to add a short comment describing the pattern you're matching (like `Match A - (A/B)*B`).


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12178
+  const auto *Add = dyn_cast<SCEVAddExpr>(Expr);
+  if (Add == nullptr || Add->getNumOperands() != 2) {
+    return false;
----------------
(Here and below) LLVM style avoids braces around single statement blocks.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12186
+    }
+    const auto MatchMulends = [&](const SCEV *B) {
+      // (SomeExpr + ((SomeExpr / B) * -B)).
----------------
I think the right term is "multiplicand", though I'd be fine with `MatchMulOperands` too. :)


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12195
+      // (SomeExpr + (-(SomeExpr / B) * B)).
+      if (HasSameValue(Expr, getURemExpr(A, B))) {
+        LHS = A;
----------------
Do you have a test case that breaks if we remove `HasEqualValue` and instead just check for pointer equality?  If not, I'd suggest removing it for now.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:12212
+  };
+  return MatchAddends(Add->getOperand(1), Add->getOperand(0)) ||
+         MatchAddends(Add->getOperand(0), Add->getOperand(1));
----------------
SCEV sorts operands by complexity and I think in your case of `A - (A/B)*B` the `A` part should always be less complex than `(A/B)*B`.  Can you check if this is the case in your test cases?  If yes, we can probably drop one of the calls to `MatchAddends`.


================
Comment at: llvm/test/Analysis/ScalarEvolution/trip-count14.ll:38
 ; CHECK-LABEL: Determining loop execution counts for: @s32_max2
-; CHECK-NEXT: Loop %do.body: backedge-taken count is ((-1 * %n) + ((2 + %n) smax %n))
+; CHECK-NEXT: Loop %do.body: backedge-taken count is ((-1 * %n)<nsw> + ((2 + %n) smax %n))
 ; CHECK-NEXT: Loop %do.body: max backedge-taken count is 2, actual taken count either this or zero.
----------------
Just picking at random -- this one doesn't look obviously correct to me -- I don't immediately see a reason why `%n` can't be `SIGNED_INT32_MIN` (if `%n` is `SIGNED_INT32_MIN` then `%n * -1` sign overflows).


https://reviews.llvm.org/D48338