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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:940
       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";
----------------
This doesn't look correct. `RightSCEV + Step - 1 = Max` means that `RightSCEV + Step` exceeds `Max`. We should bail with SLT/UGT as well. Please correct me if I'm wrong.

And in any case, if you still think it should be done, it should go as a separate patch.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1474
+      if (CannotBeMinInLoop(*SR.HighLimit, &OriginalLoop, SE,
+                            IsSignedPredicate)) {
+        ExitPreLoopAtSCEV = SE.getAddExpr(*SR.HighLimit, MinusOneS);
----------------
{ } not needed here (yes, I hate this code style rule, too :)).


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1502
+      if (CannotBeMinInLoop(*SR.LowLimit, &OriginalLoop, SE,
+                            IsSignedPredicate)) {
+        ExitMainLoopAtSCEV = SE.getAddExpr(*SR.LowLimit, MinusOneS);
----------------
Same.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1804
   }
+
   LoopStructure LS = MaybeLoopStructure.getValue();
----------------
What was the reason of that?


================
Comment at: test/Transforms/IRCE/eq_ne.ll:147
 entry:
-  %len = load i32, i32* %a_len_ptr, !range !0
   br label %loop
----------------
samparker wrote:
> mkazantsev wrote:
> > 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.
> From the description, the existing test didn't seem like it was testing what it should have been. Given the range values, IRCE should be able to be performed and with this patch it does. So I wanted to change the test to match up with the description with IRCE being prevented.
I would still prefer to not touch old tests for sake of this. Please create a new one, it doesn't hurt anyone and will help us to make sure that everything works OK if the range is there.


================
Comment at: test/Transforms/IRCE/variable-loop-bounds.ll:13
+for.body:
+  %i.017 = phi i32 [ %inc, %for.inc ], [ 0, %entry ]
+  %cmp1 = icmp ult i32 %i.017, 512
----------------
Could you please rename it to `%iv` for better readability?


https://reviews.llvm.org/D44515