From 1ca1d278d16d1f544f8e931d4f319ff58b582aca Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Fri, 27 Jan 2017 23:26:27 +0000 Subject: [PATCH] [InstCombine] move icmp transforms that might be recognized as min/max and inf-loop (PR31751) This is a minimal patch to avoid the infinite loop in: https://llvm.org/bugs/show_bug.cgi?id=31751 But the general problem is bigger: we're not canonicalizing all of the min/max forms reported by value tracking's matchSelectPattern(), and we don't define min/max consistently. Some code uses matchSelectPattern(), other code uses matchers like m_Umax, and others have their own inline definitions which may be subtly different from any of the above. The reason that the test cases in this patch need a cast op to trigger is because we don't (yet) canonicalize all min/max forms based on matchSelectPattern() in canonicalizeMinMaxWithConstant(), but we do make min/max+cast transforms based on matchSelectPattern() in visitSelectInst(). The location of the icmp transforms that trigger the inf-loop seems arbitrary at best, so I'm moving those behind the min/max fence in visitICmpInst() as the quick fix. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@293345 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/InstCombine/InstCombineCompares.cpp | 31 +++++--- test/Transforms/InstCombine/minmax-fold.ll | 82 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 10 deletions(-) diff --git a/lib/Transforms/InstCombine/InstCombineCompares.cpp b/lib/Transforms/InstCombine/InstCombineCompares.cpp index 3ebf719fceb..3a1be81b7f5 100644 --- a/lib/Transforms/InstCombine/InstCombineCompares.cpp +++ b/lib/Transforms/InstCombine/InstCombineCompares.cpp @@ -4088,11 +4088,6 @@ Instruction *InstCombiner::foldICmpUsingKnownBits(ICmpInst &I) { Constant *CMinus1 = ConstantInt::get(Op0->getType(), *CmpC - 1); return new ICmpInst(ICmpInst::ICMP_EQ, Op0, CMinus1); } - // (x (x >s -1) -> true if sign bit clear - if (CmpC->isMinSignedValue()) { - Constant *AllOnes = Constant::getAllOnesValue(Op0->getType()); - return new ICmpInst(ICmpInst::ICMP_SGT, Op0, AllOnes); - } } break; } @@ -4112,11 +4107,6 @@ Instruction *InstCombiner::foldICmpUsingKnownBits(ICmpInst &I) { if (*CmpC == Op0Max - 1) return new ICmpInst(ICmpInst::ICMP_EQ, Op0, ConstantInt::get(Op1->getType(), *CmpC + 1)); - - // (x >u 2147483647) -> (x true if sign bit set - if (CmpC->isMaxSignedValue()) - return new ICmpInst(ICmpInst::ICMP_SLT, Op0, - Constant::getNullValue(Op0->getType())); } break; } @@ -4348,6 +4338,27 @@ Instruction *InstCombiner::visitICmpInst(ICmpInst &I) { (SI->getOperand(2) == Op0 && SI->getOperand(1) == Op1)) return nullptr; + // FIXME: We only do this after checking for min/max to prevent infinite + // looping caused by a reverse canonicalization of these patterns for min/max. + // FIXME: The organization of folds is a mess. These would naturally go into + // canonicalizeCmpWithConstant(), but we can't move all of the above folds + // down here after the min/max restriction. + ICmpInst::Predicate Pred = I.getPredicate(); + const APInt *C; + if (match(Op1, m_APInt(C))) { + // For i32: x >u 2147483647 -> x true if sign bit set + if (Pred == ICmpInst::ICMP_UGT && C->isMaxSignedValue()) { + Constant *Zero = Constant::getNullValue(Op0->getType()); + return new ICmpInst(ICmpInst::ICMP_SLT, Op0, Zero); + } + + // For i32: x x >s -1 -> true if sign bit clear + if (Pred == ICmpInst::ICMP_ULT && C->isMinSignedValue()) { + Constant *AllOnes = Constant::getAllOnesValue(Op0->getType()); + return new ICmpInst(ICmpInst::ICMP_SGT, Op0, AllOnes); + } + } + if (Instruction *Res = foldICmpInstWithConstant(I)) return Res; diff --git a/test/Transforms/InstCombine/minmax-fold.ll b/test/Transforms/InstCombine/minmax-fold.ll index adb99c283c2..a9a824ed2fe 100644 --- a/test/Transforms/InstCombine/minmax-fold.ll +++ b/test/Transforms/InstCombine/minmax-fold.ll @@ -410,3 +410,85 @@ define i32 @clamp_unsigned2(i32 %x) { ret i32 %r } +; The next 3 min tests should canonicalize to the same form...and not infinite loop. + +define double @PR31751_umin1(i32 %x) { +; CHECK-LABEL: @PR31751_umin1( +; CHECK-NEXT: [[TMP1:%.*]] = icmp ult i32 %x, 2147483647 +; CHECK-NEXT: [[CONV1:%.*]] = select i1 [[TMP1]], i32 %x, i32 2147483647 +; CHECK-NEXT: [[TMP2:%.*]] = sitofp i32 [[CONV1]] to double +; CHECK-NEXT: ret double [[TMP2]] +; + %cmp = icmp slt i32 %x, 0 + %sel = select i1 %cmp, i32 2147483647, i32 %x + %conv = sitofp i32 %sel to double + ret double %conv +} + +define double @PR31751_umin2(i32 %x) { +; CHECK-LABEL: @PR31751_umin2( +; CHECK-NEXT: [[CMP:%.*]] = icmp ult i32 %x, 2147483647 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 %x, i32 2147483647 +; CHECK-NEXT: [[CONV:%.*]] = sitofp i32 [[SEL]] to double +; CHECK-NEXT: ret double [[CONV]] +; + %cmp = icmp ult i32 %x, 2147483647 + %sel = select i1 %cmp, i32 %x, i32 2147483647 + %conv = sitofp i32 %sel to double + ret double %conv +} + +define double @PR31751_umin3(i32 %x) { +; CHECK-LABEL: @PR31751_umin3( +; CHECK-NEXT: [[TMP1:%.*]] = icmp ult i32 %x, 2147483647 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[TMP1]], i32 %x, i32 2147483647 +; CHECK-NEXT: [[CONV:%.*]] = sitofp i32 [[SEL]] to double +; CHECK-NEXT: ret double [[CONV]] +; + %cmp = icmp ugt i32 %x, 2147483647 + %sel = select i1 %cmp, i32 2147483647, i32 %x + %conv = sitofp i32 %sel to double + ret double %conv +} + +; The next 3 max tests should canonicalize to the same form...and not infinite loop. + +define double @PR31751_umax1(i32 %x) { +; CHECK-LABEL: @PR31751_umax1( +; CHECK-NEXT: [[TMP1:%.*]] = icmp ugt i32 %x, -2147483648 +; CHECK-NEXT: [[CONV1:%.*]] = select i1 [[TMP1]], i32 %x, i32 -2147483648 +; CHECK-NEXT: [[TMP2:%.*]] = sitofp i32 [[CONV1]] to double +; CHECK-NEXT: ret double [[TMP2]] +; + %cmp = icmp sgt i32 %x, -1 + %sel = select i1 %cmp, i32 2147483648, i32 %x + %conv = sitofp i32 %sel to double + ret double %conv +} + +define double @PR31751_umax2(i32 %x) { +; CHECK-LABEL: @PR31751_umax2( +; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i32 %x, -2147483648 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 %x, i32 -2147483648 +; CHECK-NEXT: [[CONV:%.*]] = sitofp i32 [[SEL]] to double +; CHECK-NEXT: ret double [[CONV]] +; + %cmp = icmp ugt i32 %x, 2147483648 + %sel = select i1 %cmp, i32 %x, i32 2147483648 + %conv = sitofp i32 %sel to double + ret double %conv +} + +define double @PR31751_umax3(i32 %x) { +; CHECK-LABEL: @PR31751_umax3( +; CHECK-NEXT: [[TMP1:%.*]] = icmp ugt i32 %x, -2147483648 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[TMP1]], i32 %x, i32 -2147483648 +; CHECK-NEXT: [[CONV:%.*]] = sitofp i32 [[SEL]] to double +; CHECK-NEXT: ret double [[CONV]] +; + %cmp = icmp ult i32 %x, 2147483648 + %sel = select i1 %cmp, i32 2147483648, i32 %x + %conv = sitofp i32 %sel to double + ret double %conv +} + -- 2.11.0