[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 accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

LGTM with comments addressed.



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:344
+  SelectInst *Sel =
+      SelectInst::Create(ICmp, ConstantInt::get(T, 0), N, "tmp", Rem);
+  Rem->replaceAllUsesWith(Sel);
----------------
I'd prefer to give this a more specific name -- how about `"iv.rem"`?  The exact name does not matter as long as folks can grep the symbol name in LLVM and find this file.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:366
+  const SCEV *N = SE->getSCEV(NValue);
+  const SCEV *D = SE->getSCEV(Rem->getOperand(1));
 
----------------
Minor thing: I'd have written this as:

```
const SCEV *N = SE->getSCEVAtScope(SE->getSCEV(NValue), ICmpLoop);

// Later, after the check on !UsedAsNumerator && !IsSigned
const SCEV *D = SE->getSCEVAtScope(SE->getSCEV(DValue), ICmpLoop);
```


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:401
+  replaceSRemWithURem(Rem);
+  // Only propagate the change if IVOperand is used as numerator
+  Changed = UsedAsNumerator;
----------------
I'm not sure if this is safe -- it seems like `Changed` is piped all the way through to `IndVarSimplify`'s  return value to the pass manager (if nothing changed, it will tell the PM that all analyses are preserved).

Even if this is safe, I suspect you'll need to do `Changed |= UsedAsNumerator`.  In this case, please add a one liner on why this is safe.



Repository:
  rL LLVM

https://reviews.llvm.org/D38072