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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:311
   Value *RHS = ICI->getOperand(1);
+  DEBUG(dbgs() << "irce: parse range check on "; ICI->dump());
 
----------------
Please submit it as a separate patch.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:705
 
 static bool CanBeMin(ScalarEvolution &SE, const SCEV *S, bool Signed) {
   APInt Min = Signed ?
----------------
Can we now remove this function and replace its usages with `LoopGuardedAgainstMin`? I don't mind if it goes as a follow-up change.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:713
 
+static bool LoopGuardedAgainstMin(Loop *L, ScalarEvolution &SE, const SCEV *S,
+                                  bool Signed) {
----------------
I would suggest calling it `CannotBeMinInLoop` and reorder args like `S, L, Signed, SE`.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:878
 
+  DEBUG(dbgs() << "irce: IndVarBase = "; IndVarBase->dump());
   const SCEV *StartNext = IndVarBase->getStart();
----------------
Please submit it as a separate change.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:908
+        // IndVarStart is non negative and we're increasing with no wrap.
+        if (SE.isKnownNonNegative(IndVarStart) &&
+            IndVarBase->getNoWrapFlags(SCEV::FlagNUW) &&
----------------
Why do you need non-negativity here? In unsigned, "negative" only means that the first bit is 1 and the value is still seen as big positive.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:951
       if (SumCanReachMax(SE, RightSCEV, StepMinusOne, IsSignedPredicate)) {
-        // TODO: this restriction is easily removable -- we just have to
-        // remember that the icmp was an slt and not an sle.
-        FailureReason = "limit may overflow when coercing le to lt";
-        return None;
+        if (Pred == ICmpInst::ICMP_SLE || Pred == ICmpInst::ICMP_ULE) {
+          FailureReason = "limit may overflow when coercing le to lt";
----------------
Please submit it as a separate patch.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:991
         Pred = ICmpInst::ICMP_SGT;
       else if (Pred == ICmpInst::ICMP_EQ && LatchBrExitIdx == 0 &&
                !CanBeMax(SE, RightSCEV, /* IsSignedPredicate */ true)) {
----------------
Do you plan to do the same for max value? I'm OK if it will be done in a separate patch.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1787
 
-  if (RangeChecks.empty())
+  if (RangeChecks.empty()) {
+    DEBUG(dbgs() << "irce: empty range checks.\n");
----------------
Please submit it as a separate patch.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1813
   }
+  DEBUG(dbgs() << "irce: loop structure parsed.\n");
+
----------------
Same.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1847
 
-  if (!SafeIterRange.hasValue())
+  if (!SafeIterRange.hasValue()) {
+    DEBUG(dbgs() << "irce: unsafe iterator range.\n");
----------------
Same.


================
Comment at: test/Transforms/IRCE/eq_ne.ll:147
 entry:
-  %len = load i32, i32* %a_len_ptr, !range !0
   br label %loop
----------------
What is the reason of this change? If it demonstrates something new you can just create a new test identical to this one but without the range.


https://reviews.llvm.org/D44515