From dc1b8f42713b3ec1a8ecbdedb61ae9a63393db20 Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Tue, 2 May 2017 15:31:40 +0000 Subject: [PATCH] [InstCombine] don't use DeMorgan's Law on integer constants (2nd try) This was originally checked in here: https://reviews.llvm.org/rL301923 And reverted here: https://reviews.llvm.org/rL301924 Because there's a clang test that would fail after this. I fixed/removed the offending CHECK lines in: https://reviews.llvm.org/rL301928 So let's try this again. Original commit message: This is the fold that causes the infinite loop in BoringSSL (https://github.com/google/boringssl/blob/master/crypto/cipher/e_rc2.c) when we fix instcombine demanded bits to prefer 'not' ops as in https://reviews.llvm.org/D32255. There are 2 or 3 problems with dyn_castNotVal, and I don't think we can reinstate https://reviews.llvm.org/D32255 until dyn_castNotVal is completely eliminated. 1. As shown here, it transforms 'not' into random xor. This transform is harmful to SCEV and codegen because 'not' can often be folded while random xor cannot. 2. It does not transform vector constants. This is actually a good thing, but if you don't believe the above argument, then we shouldn't have excluded vectors. 3. It tries to avoid transforming not(not(X)). That's nice, but it doesn't match the greedy nature of instcombine. If we DeMorganize a pattern that has an extra 'not' in it: ~(~(~X) & Y) --> (~X | ~Y) That's just another case of DeMorgan, so we should trust that we'll fold that pattern too: (~X | ~ Y) --> ~(X & Y) Differential Revision: https://reviews.llvm.org/D32665 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@301929 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | 39 ++++++++++++---------- test/Transforms/InstCombine/assume2.ll | 8 ++--- test/Transforms/InstCombine/demorgan.ll | 12 +++---- 3 files changed, 31 insertions(+), 28 deletions(-) diff --git a/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp index 41ae37ee127..c7092bf3a39 100644 --- a/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp +++ b/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp @@ -2433,29 +2433,32 @@ Instruction *InstCombiner::visitXor(BinaryOperator &I) { if (Value *V = SimplifyBSwap(I)) return replaceInstUsesWith(I, V); + // Apply DeMorgan's Law for 'nand' / 'nor' logic with an inverted operand. + Value *X, *Y; + + // We must eliminate the and/or (one-use) for these transforms to not increase + // the instruction count. + // ~(~X & Y) --> (X | ~Y) + // ~(Y & ~X) --> (X | ~Y) + if (match(&I, m_Not(m_OneUse(m_c_And(m_Not(m_Value(X)), m_Value(Y)))))) { + Value *NotY = Builder->CreateNot(Y, Y->getName() + ".not"); + return BinaryOperator::CreateOr(X, NotY); + } + // ~(~X | Y) --> (X & ~Y) + // ~(Y | ~X) --> (X & ~Y) + if (match(&I, m_Not(m_OneUse(m_c_Or(m_Not(m_Value(X)), m_Value(Y)))))) { + Value *NotY = Builder->CreateNot(Y, Y->getName() + ".not"); + return BinaryOperator::CreateAnd(X, NotY); + } + // Is this a 'not' (~) fed by a binary operator? BinaryOperator *NotOp; if (match(&I, m_Not(m_BinOp(NotOp)))) { if (NotOp->getOpcode() == Instruction::And || NotOp->getOpcode() == Instruction::Or) { - // We must eliminate the and/or for this transform to not increase the - // instruction count. - if (NotOp->hasOneUse()) { - // ~(~X & Y) --> (X | ~Y) - De Morgan's Law - // ~(~X | Y) === (X & ~Y) - De Morgan's Law - if (dyn_castNotVal(NotOp->getOperand(1))) - NotOp->swapOperands(); - if (Value *Op0NotVal = dyn_castNotVal(NotOp->getOperand(0))) { - Value *NotY = Builder->CreateNot( - NotOp->getOperand(1), NotOp->getOperand(1)->getName() + ".not"); - if (NotOp->getOpcode() == Instruction::And) - return BinaryOperator::CreateOr(Op0NotVal, NotY); - return BinaryOperator::CreateAnd(Op0NotVal, NotY); - } - } - - // ~(X & Y) --> (~X | ~Y) - De Morgan's Law - // ~(X | Y) === (~X & ~Y) - De Morgan's Law + // Apply DeMorgan's Law when inverts are free: + // ~(X & Y) --> (~X | ~Y) + // ~(X | Y) --> (~X & ~Y) if (IsFreeToInvert(NotOp->getOperand(0), NotOp->getOperand(0)->hasOneUse()) && IsFreeToInvert(NotOp->getOperand(1), diff --git a/test/Transforms/InstCombine/assume2.ll b/test/Transforms/InstCombine/assume2.ll index e8fbc049f41..8dc8831fffa 100644 --- a/test/Transforms/InstCombine/assume2.ll +++ b/test/Transforms/InstCombine/assume2.ll @@ -21,8 +21,8 @@ define i32 @test1(i32 %a) #0 { define i32 @test2(i32 %a) #0 { ; CHECK-LABEL: @test2( -; CHECK-NEXT: [[A_NOT:%.*]] = or i32 [[A:%.*]], -16 -; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[A_NOT]], -6 +; CHECK-NEXT: [[AND:%.*]] = and i32 [[A:%.*]], 15 +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[AND]], 10 ; CHECK-NEXT: tail call void @llvm.assume(i1 [[CMP]]) ; CHECK-NEXT: ret i32 2 ; @@ -50,8 +50,8 @@ define i32 @test3(i32 %a) #0 { define i32 @test4(i32 %a) #0 { ; CHECK-LABEL: @test4( -; CHECK-NEXT: [[A_NOT:%.*]] = and i32 [[A:%.*]], 15 -; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[A_NOT]], 10 +; CHECK-NEXT: [[V:%.*]] = or i32 [[A:%.*]], -16 +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[V]], -6 ; CHECK-NEXT: tail call void @llvm.assume(i1 [[CMP]]) ; CHECK-NEXT: ret i32 2 ; diff --git a/test/Transforms/InstCombine/demorgan.ll b/test/Transforms/InstCombine/demorgan.ll index fc4af3c6dad..26c2270a3fd 100644 --- a/test/Transforms/InstCombine/demorgan.ll +++ b/test/Transforms/InstCombine/demorgan.ll @@ -367,12 +367,12 @@ define i8 @demorgan_nor_use2bc(i8 %A, i8 %B) { ret i8 %r2 } -; FIXME: Do not apply DeMorgan's Law to constants. We prefer 'not' ops. +; Do not apply DeMorgan's Law to constants. We prefer 'not' ops. define i32 @demorganize_constant1(i32 %a) { ; CHECK-LABEL: @demorganize_constant1( -; CHECK-NEXT: [[A_NOT:%.*]] = or i32 %a, -16 -; CHECK-NEXT: [[AND1:%.*]] = xor i32 [[A_NOT]], 15 +; CHECK-NEXT: [[AND:%.*]] = and i32 %a, 15 +; CHECK-NEXT: [[AND1:%.*]] = xor i32 [[AND]], -1 ; CHECK-NEXT: ret i32 [[AND1]] ; %and = and i32 %a, 15 @@ -380,12 +380,12 @@ define i32 @demorganize_constant1(i32 %a) { ret i32 %and1 } -; FIXME: Do not apply DeMorgan's Law to constants. We prefer 'not' ops. +; Do not apply DeMorgan's Law to constants. We prefer 'not' ops. define i32 @demorganize_constant2(i32 %a) { ; CHECK-LABEL: @demorganize_constant2( -; CHECK-NEXT: [[A_NOT:%.*]] = and i32 %a, -16 -; CHECK-NEXT: [[AND1:%.*]] = xor i32 [[A_NOT]], -16 +; CHECK-NEXT: [[AND:%.*]] = or i32 %a, 15 +; CHECK-NEXT: [[AND1:%.*]] = xor i32 [[AND]], -1 ; CHECK-NEXT: ret i32 [[AND1]] ; %and = or i32 %a, 15 -- 2.11.0