[PATCH] D145230: [ScalarEvolution] Apply loop guards against min/max for its arguments

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Underlying code seems buggy to me. I'd prefer it fixed in follow-ups.

Your change mostly LG, if possible, do variable renaming as a separate patch.

Please wait couple of days in case if someone else has concerns before landing this.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15074-15082
+      case CmpInst::ICMP_ULT:
+        if (RHS->getType()->isPointerTy())
+          return;
+        RHS = getUMaxExpr(RHS, One);
+        LLVM_FALLTHROUGH;
+      case CmpInst::ICMP_SLT: {
+        RHS = getMinusSCEV(RHS, One);
----------------
I know it's not your checks, but this can be changed further.
- Why pointers are checked only for `ult`? `ugt` will also make us computations with it, and why not check it for signed?
-- Does this whole thing even make any sense for pointers?
- Why we do `umax(..., UMIN + 1)` correction for unsigned but not `smax(..., SMIN + 1)` for signed? Why don't we do the same correction for `umin(RHS, UMAX - 1)` when we are about to add `one`? Looks like an attempt to hide some bugs.





================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:15104
+        continue;
+      const SCEV *FromRewritten = GetMaybeRewritten(From);
+      const SCEV *To = nullptr;
----------------
Can variable renaming be done separately?


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

https://reviews.llvm.org/D145230