[IRCE] Temporarily disable unsigned latch conditions by default

[mkazantsev]

Summary

We have found some corner cases connected to range intersection where IRCE makes
a bad thing when the latch condition is unsigned. The fix for that will go as a follow up.
This patch temporarily disables IRCE for unsigned latch conditions until the issue is fixed.

The unsigned latch conditions were introduced to IRCE by rL310027.

[reames]

LGTM w/the following changes made: add a test case which demonstrates the miscompile and update the description to reference the original problematic commit.

[reames]

llvm/trunk/test/Transforms/IRCE/range_intersect_miscompile.ll

3 ↗

(On Diff #117631)

This comment needs clarified. It reads as if this is an active miscompile. After you disabled the unsigned latch case, this is no longer active.

Another way of structuring this would be to write the test using the off-by-default output and check the incorrect output matches what you expect. Then you'd have a test case to edit to show the effect of fixing the miscompile when you re-enable the unsigned latch.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp

static cl::opt<bool> PrintRangeChecks("irce-print-range-checks", cl::Hidden,
                                       cl::init(false));
 
 static cl::opt<int> MaxExitProbReciprocal("irce-max-exit-prob-reciprocal",
                                           cl::Hidden, cl::init(10));
 
 static cl::opt<bool> SkipProfitabilityChecks("irce-skip-profitability-checks",
                                              cl::Hidden, cl::init(false));
 
+static cl::opt<bool> AllowUnsignedLatchCondition("irce-allow-unsigned-latch",
+                                                 cl::Hidden, cl::init(false));
+
 static const char *ClonedLoopTag = "irce.loop.clone";
 
 #define DEBUG_TYPE "irce"
 
 namespace {
 
 /// An inductive range check is conditional branch in a loop with
 ///
if (IsIncreasing) {
 
     if (!FoundExpectedPred) {
       FailureReason = "expected icmp slt semantically, found something else";
       return None;
     }
 
     IsSignedPredicate =
         Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGT;
+
+    // FIXME: We temporarily disable unsigned latch conditions by default
+    // because of found problems with intersecting signed and unsigned ranges.
+    // We are going to turn it on once the problems are fixed.
+    if (!IsSignedPredicate && !AllowUnsignedLatchCondition) {
+      FailureReason = "unsigned latch conditions are explicitly prohibited";
+      return None;
+    }
+
     // The predicate that we need to check that the induction variable lies
     // within bounds.
     ICmpInst::Predicate BoundPred =
         IsSignedPredicate ? CmpInst::ICMP_SLT : CmpInst::ICMP_ULT;
 
     if (LatchBrExitIdx == 0) {
       const SCEV *StepMinusOne = SE.getMinusSCEV(Step,
                                                  SE.getOne(Step->getType()));
if (IsIncreasing) {
 
     if (!FoundExpectedPred) {
       FailureReason = "expected icmp sgt semantically, found something else";
       return None;
     }
 
     IsSignedPredicate =
         Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGT;
+
+    // FIXME: We temporarily disable unsigned latch conditions by default
+    // because of found problems with intersecting signed and unsigned ranges.
+    // We are going to turn it on once the problems are fixed.
+    if (!IsSignedPredicate && !AllowUnsignedLatchCondition) {
+      FailureReason = "unsigned latch conditions are explicitly prohibited";
+      return None;
+    }
+
     // The predicate that we need to check that the induction variable lies
     // within bounds.
     ICmpInst::Predicate BoundPred =
         IsSignedPredicate ? CmpInst::ICMP_SGT : CmpInst::ICMP_UGT;
 
     if (LatchBrExitIdx == 0) {
       const SCEV *StepPlusOne = SE.getAddExpr(Step, SE.getOne(Step->getType()));
       if (SumCanReachMin(SE, RightSCEV, StepPlusOne, IsSignedPredicate)) {

test/Transforms/IRCE/clamp.ll

-; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s
+; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
 
 ; The test demonstrates that incorrect behavior of Clamp may lead to incorrect
 ; calculation of post-loop exit condition.
 
 ; CHECK: irce: in function test: constrained Loop at depth 1 containing: %loop<header><exiting>,%in_bounds<exiting>,%not_zero<latch><exiting>
 
 define void @test() {
 entry:

test/Transforms/IRCE/eq_ne.ll

-; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s
+; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
 
 ; CHECK: irce: in function test_01: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_01u: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_02: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_03: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_04: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_05: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_06: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>

test/Transforms/IRCE/range_intersect_miscompile.ll

+; RUN: opt -irce -S < %s 2>&1 | FileCheck %s
+
+; This test demonstrates a miscompile: the outer loop's IV iterates in range of
+; [2, 400) and the range check is done against value 331. Due to a bug in range
+; intersection IRCE manages to eliminate the range check without inserting a
+; postloop, which is incorrect. So far IRCE is prohibited for this case.
+
+define void @test_01() {
+
+; CHECK-LABEL: test_01
+; CHECK-NOT:   br i1 true
+
+entry:
+  br label %loop_header
+
+loop_header:                            ; preds = %loop_latch, %entry
+  %iv = phi i32 [ 2, %entry ], [ %iv_next, %loop_latch ]
+  %iv.prev = phi i32 [ 1, %entry ], [ %iv, %loop_latch ]
+  %tmp2 = icmp sgt i32 %iv.prev, -1
+  br i1 %tmp2, label %loop_header.split.us, label %exit
+
+loop_header.split.us:                   ; preds = %loop_header
+  br label %inner_loop
+
+inner_loop:                                   ; preds = %inner_loop, %loop_header.split.us
+  %inner_iv = phi i32 [ 1, %loop_header.split.us ], [ %inner_iv_next, %inner_loop ]
+  %inner_iv_next = add nuw nsw i32 %inner_iv, 1
+  %inner_cond = icmp ult i32 %inner_iv_next, 31
+  br i1 %inner_cond, label %inner_loop, label %range_check_block
+
+exit:                                            ; preds = %loop_latch, %loop_header
+  ret void
+
+range_check_block:                                          ; preds = %inner_loop
+  %range_check = icmp slt i32 %iv, 331
+  br i1 %range_check, label %loop_latch, label %deopt
+
+loop_latch:                                         ; preds = %range_check_block
+  %iv_next = add i32 %iv, 1
+  %loop_cond = icmp ult i32 %iv_next, 400
+  br i1 %loop_cond, label %loop_header, label %exit
+
+deopt:                                          ; preds = %range_check_block
+  ret void
+}

test/Transforms/IRCE/stride_more_than_1.ll

-; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s
+; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
 
 ; CHECK: irce: in function test_01: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_02: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_03: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_04: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_05: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_06: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_07: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>

test/Transforms/IRCE/unsigned_comparisons_ugt.ll

-; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s
+; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
 
 ; CHECK: irce: in function test_01: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_02: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_03: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_04: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_05: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_06: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 

test/Transforms/IRCE/unsigned_comparisons_ult.ll

-; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s
+; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
 
 ; CHECK: irce: in function test_01: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_02: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_03: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_04: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_05: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK: irce: in function test_06: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
 ; CHECK-NOT: irce: in function test_07: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>