From 4e9d6cd35404b8804150c43ea7a8375f398664f4 Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Wed, 9 Nov 2016 00:13:11 +0000 Subject: [PATCH] [InstCombine] fix profitability equation for max-of-nots transform As the test change shows, we can increase the critical path by adding a 'not' instruction, so make sure that we're actually removing an instruction if we do this transform. This transform could also cause us to miss folds of min/max pairs. llvm-svn: 286315 --- llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp | 13 ++++++------- llvm/test/Transforms/InstCombine/max-of-nots.ll | 12 +++++++----- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp index 6bee87c..06b769b 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp @@ -1308,15 +1308,14 @@ Instruction *InstCombiner::visitSelectInst(SelectInst &SI) { if ((SPF == SPF_SMAX || SPF == SPF_UMAX) && IsFreeToInvert(LHS, LHS->hasNUses(2)) && IsFreeToInvert(RHS, RHS->hasNUses(2))) { - // This transform adds a not operation, and that extra cost needs to be - // justified. We look for simplifications that will result from applying - // this rule: - bool Profitable = - (LHS->hasNUses(2) && match(LHS, m_Not(m_Value()))) || - (RHS->hasNUses(2) && match(RHS, m_Not(m_Value()))) || + // For this transform to be profitable, we need to eliminate at least two + // 'not' instructions if we're going to add one 'not' instruction. + int NumberOfNots = + (LHS->hasNUses(2) && match(LHS, m_Not(m_Value()))) + + (RHS->hasNUses(2) && match(RHS, m_Not(m_Value()))) + (SI.hasOneUse() && match(*SI.user_begin(), m_Not(m_Value()))); - if (Profitable) { + if (NumberOfNots >= 2) { Value *NewLHS = Builder->CreateNot(LHS); Value *NewRHS = Builder->CreateNot(RHS); Value *NewCmp = SPF == SPF_SMAX diff --git a/llvm/test/Transforms/InstCombine/max-of-nots.ll b/llvm/test/Transforms/InstCombine/max-of-nots.ll index ca9520a..96fac52 100644 --- a/llvm/test/Transforms/InstCombine/max-of-nots.ll +++ b/llvm/test/Transforms/InstCombine/max-of-nots.ll @@ -34,13 +34,15 @@ define i32 @compute_min_3(i32 %x, i32 %y, i32 %z) { ret i32 %min } +; Don't increase the critical path by moving the 'not' op after the 'select'. + define i32 @compute_min_arithmetic(i32 %x, i32 %y) { ; CHECK-LABEL: @compute_min_arithmetic( -; CHECK-NEXT: [[TMP1:%.*]] = add i32 %x, -4 -; CHECK-NEXT: [[TMP2:%.*]] = icmp slt i32 [[TMP1]], %y -; CHECK-NEXT: [[TMP3:%.*]] = select i1 [[TMP2]], i32 [[TMP1]], i32 %y -; CHECK-NEXT: [[TMP4:%.*]] = xor i32 [[TMP3]], -1 -; CHECK-NEXT: ret i32 [[TMP4]] +; CHECK-NEXT: [[NOT_VALUE:%.*]] = sub i32 3, %x +; CHECK-NEXT: [[NOT_Y:%.*]] = xor i32 %y, -1 +; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i32 [[NOT_VALUE]], [[NOT_Y]] +; CHECK-NEXT: [[NOT_MIN:%.*]] = select i1 [[CMP]], i32 [[NOT_VALUE]], i32 [[NOT_Y]] +; CHECK-NEXT: ret i32 [[NOT_MIN]] ; %not_value = sub i32 3, %x %not_y = sub i32 -1, %y -- 2.7.4