[PATCH] D38072: [SimplifyIndvar] Replace the srem 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.

This is looking close; only some nitpicky comments inline



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:340
+                                   const SCEV *SD) {
+  if (!SE->isKnownNonNegative(SN) || !SE->isKnownNonNegative(SD))
+    return false;
----------------
Can we share this work amongst the three helpers?  Something like:

 - Pass in `NumeratorNonNegative` and `DenominatorNonNegative` bools to this function, and only `NumeratorNonNegative` to the other two.
 - Compute these bools in `simplifyIVRemainder` as needed


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:373
+  const auto *LessOne = SE->getMinusSCEV(SN, SE->getOne(T));
+  if (!isAlwaysLessThanAndNonNegative(LessOne, SD, IsSigned))
+    return false;
----------------
This specific function doesn't add much IMHO -- I'd just inline it out.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:402
+  const SCEV *S = SE->getSCEV(N);
+  const SCEV *X = SE->getSCEV(D);
 
----------------
Let's keep calling these `N` and `D` -- perhaps you could differentiate the `Value *` and the `SCEV *` by a suffix (`N` vs `NValue`)?


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:410
+  if (UsedAsNumerator) {
+    if (replaceRemByNumerator(Rem, S, X, IsSigned))
       return;
----------------
s/By/With/ in the names


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:417
 
-  DEBUG(dbgs() << "INDVARS: Simplified rem: " << *Rem << '\n');
-  ++NumElimRem;
-  Changed = true;
-  DeadInsts.emplace_back(Rem);
+  if (IsSigned && eliminateSRem(Rem, S, X))
+    // Only propagate the change if IVOperand is used as numerator
----------------
Going by the other function names, how about calling this `replaceSRemWithURem`?


Repository:
  rL LLVM

https://reviews.llvm.org/D38072