[PATCH] D31488: [SimplifyIndvar] Replace the sdiv used by IV if we can prove both of its operands are non-negative

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Comments inline.

You can also do this same rewrite within SCEV.  That is, try to have `getSCEV(sdiv instruction)` return a `SCEVUDivExpr` if legal.  However, given that `udiv` is more canonical, this patch is good too if it solves your problem.



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:38
 STATISTIC(NumElimRem     , "Number of IV remainder operations eliminated");
+STATISTIC(NumElimSDiv    , "Number of IV signed division operations eliminated");
 STATISTIC(NumElimCmp     , "Number of IV comparisons eliminated");
----------------
s/eliminated/converted to unsigned division/


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:275
+  // the numerator.
+  if (IVOperand != SDiv->getOperand(0))
+    return false;
----------------
This seems unnecessary - why do you not care about optimizing

```
%val = sdiv <non-iv-known-positive>, <iv>
```



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:279
+  // Get the SCEVs for the ICmp operands.
+  auto S = SE->getSCEV(SDiv->getOperand(0));
+  auto X = SE->getSCEV(SDiv->getOperand(1));
----------------
Use `auto *`.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:284
+  const Loop *L = LI->getLoopFor(SDiv->getParent());
+  S = SE->getSCEVAtScope(S, L);
+  X = SE->getSCEVAtScope(X, L);
----------------
I'd use `N` and `D` instead of `S` and `X`.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:289
+  if (SE->isKnownNonNegative(S) && SE->isKnownNonNegative(X)) {
+    auto UDiv = BinaryOperator::Create(
+        BinaryOperator::UDiv, SDiv->getOperand(0), SDiv->getOperand(1),
----------------
Use `auto *`.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:297
+    Changed = true;
+    DeadInsts.emplace_back(SDiv);
+    return true;
----------------
Why not `push_back`?


https://reviews.llvm.org/D31488