[PATCH] D44515: [IRCE] Change min value safety check

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Code looks fine with minor nits inlined. Please add more tests, I want to be adamant that it also works for other latch predicates.



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:688
 }
 
+static bool isSafeIncreasingBound(const SCEV *Start,
----------------
I think I got what this function does, but do you mind adding an explanatory comment since it is not completely trivial?


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:690
+static bool isSafeIncreasingBound(const SCEV *Start,
+                                  const SCEV *S1, const SCEV *Step,
+                                  ICmpInst::Predicate Pred,
----------------
Please rename `S1` to something more sound, it is unclear how it is connected to `Start` and `Step`. I propose `Bound` or something of this nature.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:708-709
+  }
+
+  const SCEV *StepMinusOne = SE.getMinusSCEV(Step,
+                                             SE.getOne(Step->getType()));
----------------
Assert that `LatchBrExitIdx == 0` at this point.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:713
+  APInt Max = IsSigned ? APInt::getSignedMaxValue(BitWidth) : 
+    APInt::getMaxValue(BitWidth);
+  const SCEV *Limit = SE.getMinusSCEV(SE.getConstant(Max), StepMinusOne);
----------------
Please clang-format this.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:716
+
+  return (SE.isAvailableAtLoopEntry(S1, L) &&
+          SE.isLoopEntryGuardedByCond(L, BoundPred, Start,
----------------
Availabllity of `S1` can be checked in very beginning since you check it for both `LatchBrExitIdx == 0` and `1`. I also think that this can actually be an assert, but not sure about the latter.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:728
+  auto Predicate = Signed ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
+  return SE.isAvailableAtLoopEntry(BoundSCEV, L) &&
+         SE.isLoopEntryGuardedByCond(L, Predicate, BoundSCEV,
----------------
This maybe also can be an assert, but not 100% sure about it. It's up to you.


================
Comment at: test/Transforms/IRCE/variable-loop-bounds.ll:37
+  %inc = add nuw nsw i32 %i.017, 1
+  %exitcond = icmp eq i32 %inc, %N
+  br i1 %exitcond, label %for.cond.cleanup, label %for.body
----------------
Do you mind adding similar tests with equivalent exit conditions defined by different predicates? (i.e. `ne`, `slt`, `ult` etc)


https://reviews.llvm.org/D44515