[IRCE] Fix buggy behavior in Clamp

[mkazantsev]

Summary

Clamp function was too optimistic when choosing signed or unsigned min/max function for calculations.
In fact, !IsSignedPredicate guarantees us that Smallest and Greatest can be compared safely using unsigned
predicates, but we did not check this for S which can in theory be negative.

This patch makes Clamp use signed min/max for cases when it fails to prove S being non-negative,
and it adds a test where such situation may lead to incorrect conditions calculation.

[anna]

LGTM w/comment.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp

1088 ↗

(On Diff #111657)

I think there is just one missed case of checking isKnownNonNegative right?

Just looked through IRCE code and we correctly check it for choosing the right predicate: ULT or SLT.

test/Transforms/IRCE/clamp.ll

9 ↗

(On Diff #111657)

Please give canonical names to the basic blocks (i.e. entry, header etc).

[mkazantsev]

Renamed test's blocks.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp

1088 ↗

(On Diff #111657)

Yes, for other values we know that they are non-negative because they are start and final values of the IV which is checked by unsigned condition. For S, we didn't check it.

llvm/trunk/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp

if (Increasing) {
     //
 
     Smallest = SE.getAddExpr(End, One);
     Greatest = SE.getAddExpr(Start, One);
     GreatestSeen = Start;
   }
 
   auto Clamp = [this, Smallest, Greatest, IsSignedPredicate](const SCEV *S) {
-    return IsSignedPredicate
+    bool MaybeNegativeValues = IsSignedPredicate || !SE.isKnownNonNegative(S);
+    return MaybeNegativeValues
                ? SE.getSMaxExpr(Smallest, SE.getSMinExpr(Greatest, S))
                : SE.getUMaxExpr(Smallest, SE.getUMinExpr(Greatest, S));
   };
 
   // In some cases we can prove that we don't need a pre or post loop.
   ICmpInst::Predicate PredLE =
       IsSignedPredicate ? ICmpInst::ICMP_SLE : ICmpInst::ICMP_ULE;
   ICmpInst::Predicate PredLT =

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

+; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -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:
+  %indvars.iv.next467 = add nuw nsw i64 2, 1
+  %length.i167 = load i32, i32 addrspace(1)* undef, align 8
+  %tmp21 = zext i32 %length.i167 to i64
+  %tmp34 = load atomic i32, i32 addrspace(1)* undef unordered, align 4
+  %tmp35 = add i32 %tmp34, -9581
+  %tmp36 = icmp ugt i32 %length.i167, 1
+  br i1 %tmp36, label %preheader, label %exit
+
+exit:                                          ; preds = %in_bounds, %loop, %not_zero, %entry
+  ret void
+
+preheader:                                 ; preds = %entry
+; CHECK:      preheader:
+; CHECK-NEXT:   %length_gep.i146 = getelementptr inbounds i8, i8 addrspace(1)* undef, i64 8
+; CHECK-NEXT:   %length_gep_typed.i147 = bitcast i8 addrspace(1)* undef to i32 addrspace(1)*
+; CHECK-NEXT:   %tmp43 = icmp ult i64 %indvars.iv.next467, %tmp21
+; CHECK-NEXT:   br i1 false, label %loop.preheader, label %main.pseudo.exit
+
+  %length_gep.i146 = getelementptr inbounds i8, i8 addrspace(1)* undef, i64 8
+  %length_gep_typed.i147 = bitcast i8 addrspace(1)* undef to i32 addrspace(1)*
+  %tmp43 = icmp ult i64 %indvars.iv.next467, %tmp21
+  br label %loop
+
+not_zero:                                       ; preds = %in_bounds
+; CHECK:      not_zero:
+; CHECK:        %tmp56 = icmp ult i64 %indvars.iv.next, %tmp21
+; CHECK-NEXT:   [[COND:%[^ ]+]] = icmp ult i64 %indvars.iv.next, 1
+; CHECK-NEXT:   br i1 [[COND]], label %loop, label %main.exit.selector
+
+  %tmp51 = trunc i64 %indvars.iv.next to i32
+  %tmp53 = mul i32 %tmp51, %tmp51
+  %tmp54 = add i32 %tmp53, -9582
+  %tmp55 = add i32 %tmp54, %tmp62
+  %tmp56 = icmp ult i64 %indvars.iv.next, %tmp21
+  br i1 %tmp56, label %loop, label %exit
+
+loop:                                       ; preds = %not_zero, %preheader
+  %tmp62 = phi i32 [ 1, %preheader ], [ %tmp55, %not_zero ]
+  %indvars.iv750 = phi i64 [ 1, %preheader ], [ %indvars.iv.next, %not_zero ]
+  %length.i148 = load i32, i32 addrspace(1)* %length_gep_typed.i147, align 8
+  %tmp68 = zext i32 %length.i148 to i64
+  %tmp97 = icmp ult i64 2, %tmp68
+  %or.cond = and i1 %tmp43, %tmp97
+  %tmp99 = icmp ult i64 %indvars.iv750, %tmp21
+  %or.cond1 = and i1 %or.cond, %tmp99
+  br i1 %or.cond1, label %in_bounds, label %exit
+
+in_bounds:                                       ; preds = %loop
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv750, 3
+  %tmp107 = icmp ult i64 %indvars.iv.next, 2
+  br i1 %tmp107, label %not_zero, label %exit
+}