From 2d562fe758bfc6e94c4d794bf5cbf4775854d958 Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Tue, 21 Feb 2017 19:33:53 +0000 Subject: [PATCH] [InstCombine] canonicalize non-obivous forms of integer min/max This is part of trying to clean up our handling of min/max patterns in IR. By converting these to canonical form, we're more likely to recognize them because there are various places in InstCombine that don't use matchSelectPattern or m_SMax and friends. The backend fixups referenced in the now deleted TODO comment were added with: https://reviews.llvm.org/rL291392 https://reviews.llvm.org/rL289738 If there's any codegen fallout from this change, we should be able to address it in DAGCombiner or target-specific lowering. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@295758 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/InstCombine/InstCombineSelect.cpp | 41 ++++++++++++++---------- test/Transforms/InstCombine/select.ll | 3 +- test/Transforms/InstCombine/select_meta.ll | 32 +++++++----------- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/lib/Transforms/InstCombine/InstCombineSelect.cpp b/lib/Transforms/InstCombine/InstCombineSelect.cpp index dcfacc2e7d9..b8c1daab3f6 100644 --- a/lib/Transforms/InstCombine/InstCombineSelect.cpp +++ b/lib/Transforms/InstCombine/InstCombineSelect.cpp @@ -499,18 +499,16 @@ static bool adjustMinMax(SelectInst &Sel, ICmpInst &Cmp) { return true; } -/// If this is an integer min/max where the select's 'true' operand is a -/// constant, canonicalize that constant to the 'false' operand: -/// select (icmp Pred X, C), C, X --> select (icmp Pred' X, C), X, C +/// If this is an integer min/max (icmp + select) with a constant operand, +/// create the canonical icmp for the min/max operation and canonicalize the +/// constant to the 'false' operand of the select: +/// select (icmp Pred X, C1), C2, X --> select (icmp Pred' X, C2), X, C2 +/// Note: if C1 != C2, this will change the icmp constant to the existing +/// constant operand of the select. static Instruction * canonicalizeMinMaxWithConstant(SelectInst &Sel, ICmpInst &Cmp, InstCombiner::BuilderTy &Builder) { - // TODO: We should also canonicalize min/max when the select has a different - // constant value than the cmp constant, but we need to fix the backend first. - if (!Cmp.hasOneUse() || !isa(Cmp.getOperand(1)) || - !isa(Sel.getTrueValue()) || - isa(Sel.getFalseValue()) || - Cmp.getOperand(1) != Sel.getTrueValue()) + if (!Cmp.hasOneUse() || !isa(Cmp.getOperand(1))) return nullptr; // Canonicalize the compare predicate based on whether we have min or max. @@ -525,16 +523,25 @@ canonicalizeMinMaxWithConstant(SelectInst &Sel, ICmpInst &Cmp, default: return nullptr; } - // Canonicalize the constant to the right side. - if (isa(LHS)) - std::swap(LHS, RHS); + // Is this already canonical? + if (Cmp.getOperand(0) == LHS && Cmp.getOperand(1) == RHS && + Cmp.getPredicate() == NewPred) + return nullptr; + + // Create the canonical compare and plug it into the select. + Sel.setCondition(Builder.CreateICmp(NewPred, LHS, RHS)); - Value *NewCmp = Builder.CreateICmp(NewPred, LHS, RHS); - SelectInst *NewSel = SelectInst::Create(NewCmp, LHS, RHS, "", nullptr, &Sel); + // If the select operands did not change, we're done. + if (Sel.getTrueValue() == LHS && Sel.getFalseValue() == RHS) + return &Sel; - // We swapped the select operands, so swap the metadata too. - NewSel->swapProfMetadata(); - return NewSel; + // If we are swapping the select operands, swap the metadata too. + assert(Sel.getTrueValue() == RHS && Sel.getFalseValue() == LHS && + "Unexpected results from matchSelectPattern"); + Sel.setTrueValue(LHS); + Sel.setFalseValue(RHS); + Sel.swapProfMetadata(); + return &Sel; } /// Visit a SelectInst that has an ICmpInst as its first operand. diff --git a/test/Transforms/InstCombine/select.ll b/test/Transforms/InstCombine/select.ll index cbd2123ad7d..a1ca6999f86 100644 --- a/test/Transforms/InstCombine/select.ll +++ b/test/Transforms/InstCombine/select.ll @@ -1264,11 +1264,10 @@ define i32 @PR23757(i32 %x) { define i32 @PR27137(i32 %a) { ; CHECK-LABEL: @PR27137( ; CHECK-NEXT: [[NOT_A:%.*]] = xor i32 %a, -1 -; CHECK-NEXT: [[C0:%.*]] = icmp slt i32 %a, 0 +; CHECK-NEXT: [[C0:%.*]] = icmp sgt i32 [[NOT_A]], -1 ; CHECK-NEXT: [[S0:%.*]] = select i1 [[C0]], i32 [[NOT_A]], i32 -1 ; CHECK-NEXT: ret i32 [[S0]] ; - %not_a = xor i32 %a, -1 %c0 = icmp slt i32 %a, 0 %s0 = select i1 %c0, i32 %not_a, i32 -1 diff --git a/test/Transforms/InstCombine/select_meta.ll b/test/Transforms/InstCombine/select_meta.ll index 82a85e5836d..7d5771a0a81 100644 --- a/test/Transforms/InstCombine/select_meta.ll +++ b/test/Transforms/InstCombine/select_meta.ll @@ -193,12 +193,11 @@ define i32 @test74(i32 %x) { ret i32 %retval } -; FIXME: ; The compare should change, but the metadata remains the same because the select operands are not swapped. define i32 @smin1(i32 %x) { ; CHECK-LABEL: @smin1( ; CHECK-NEXT: [[NOT_X:%.*]] = xor i32 %x, -1 -; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 %x, 0 +; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[NOT_X]], -1 ; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NOT_X]], i32 -1, !prof ![[MD1]] ; CHECK-NEXT: ret i32 [[SEL]] ; @@ -208,13 +207,12 @@ define i32 @smin1(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, and the metadata is swapped because the select operands are swapped. define i32 @smin2(i32 %x) { ; CHECK-LABEL: @smin2( ; CHECK-NEXT: [[NOT_X:%.*]] = xor i32 %x, -1 -; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 %x, 0 -; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 -1, i32 [[NOT_X]], !prof ![[MD1]] +; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[NOT_X]], -1 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NOT_X]], i32 -1, !prof ![[MD3]] ; CHECK-NEXT: ret i32 [[SEL]] ; %not_x = xor i32 %x, -1 @@ -223,12 +221,11 @@ define i32 @smin2(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, but the metadata remains the same because the select operands are not swapped. define i32 @smax1(i32 %x) { ; CHECK-LABEL: @smax1( ; CHECK-NEXT: [[NOT_X:%.*]] = xor i32 %x, -1 -; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 %x, 0 +; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 [[NOT_X]], -1 ; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NOT_X]], i32 -1, !prof ![[MD1]] ; CHECK-NEXT: ret i32 [[SEL]] ; @@ -238,13 +235,12 @@ define i32 @smax1(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, and the metadata is swapped because the select operands are swapped. define i32 @smax2(i32 %x) { ; CHECK-LABEL: @smax2( ; CHECK-NEXT: [[NOT_X:%.*]] = xor i32 %x, -1 -; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 %x, 0 -; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 -1, i32 [[NOT_X]], !prof ![[MD1]] +; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 [[NOT_X]], -1 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NOT_X]], i32 -1, !prof ![[MD3]] ; CHECK-NEXT: ret i32 [[SEL]] ; %not_x = xor i32 %x, -1 @@ -253,11 +249,10 @@ define i32 @smax2(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, but the metadata remains the same because the select operands are not swapped. define i32 @umin1(i32 %x) { ; CHECK-LABEL: @umin1( -; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 %x, -1 +; CHECK-NEXT: [[CMP:%.*]] = icmp ult i32 %x, -2147483648 ; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 %x, i32 -2147483648, !prof ![[MD1]] ; CHECK-NEXT: ret i32 [[SEL]] ; @@ -266,12 +261,11 @@ define i32 @umin1(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, and the metadata is swapped because the select operands are swapped. define i32 @umin2(i32 %x) { ; CHECK-LABEL: @umin2( -; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 %x, 0 -; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 2147483647, i32 %x, !prof ![[MD1]] +; CHECK-NEXT: [[CMP:%.*]] = icmp ult i32 %x, 2147483647 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 %x, i32 2147483647, !prof ![[MD3]] ; CHECK-NEXT: ret i32 [[SEL]] ; %cmp = icmp slt i32 %x, 0 @@ -279,11 +273,10 @@ define i32 @umin2(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, but the metadata remains the same because the select operands are not swapped. define i32 @umax1(i32 %x) { ; CHECK-LABEL: @umax1( -; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 %x, 0 +; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i32 %x, 2147483647 ; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 %x, i32 2147483647, !prof ![[MD1]] ; CHECK-NEXT: ret i32 [[SEL]] ; @@ -292,12 +285,11 @@ define i32 @umax1(i32 %x) { ret i32 %sel } -; FIXME: ; The compare should change, and the metadata is swapped because the select operands are swapped. define i32 @umax2(i32 %x) { ; CHECK-LABEL: @umax2( -; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 %x, -1 -; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 -2147483648, i32 %x, !prof ![[MD1]] +; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i32 %x, -2147483648 +; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i32 %x, i32 -2147483648, !prof ![[MD3]] ; CHECK-NEXT: ret i32 [[SEL]] ; %cmp = icmp sgt i32 %x, -1 -- 2.11.0