From 9ca8d0cce0b0a4b2464498bb61b697656aa1a4c7 Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Fri, 17 May 2019 15:52:58 +0000 Subject: [PATCH] [DAGCombiner] visitShiftByConstant(): drop bogus signbit check Summary: That check claims that the transform is illegal otherwise. That isn't true: 1. For `ISD::ADD`, we only process `ISD::SHL` outer shift => sign bit does not matter https://rise4fun.com/Alive/K4A 2. For `ISD::AND`, there is no restriction on constants: https://rise4fun.com/Alive/Wy3 3. For `ISD::OR`, there is no restriction on constants: https://rise4fun.com/Alive/GOH 3. For `ISD::XOR`, there is no restriction on constants: https://rise4fun.com/Alive/ml6 So, why is it there then? This changes the testcase that was touched by @spatel in rL347478, but i'm not sure that test tests anything particular? Reviewers: RKSimon, spatel, craig.topper, jojo, rengolin Reviewed By: spatel Subscribers: javed.absar, llvm-commits, spatel Tags: #llvm Differential Revision: https://reviews.llvm.org/D61918 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@361044 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 27 +++++++------------ .../AArch64/pull-binop-through-shift.ll | 8 +++--- test/CodeGen/X86/pull-binop-through-shift.ll | 6 ++--- test/CodeGen/X86/xor.ll | 4 +-- 4 files changed, 18 insertions(+), 27 deletions(-) diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp index 27da26446ee..f88f084dd5e 100644 --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp @@ -6582,11 +6582,16 @@ SDValue DAGCombiner::visitXOR(SDNode *N) { /// Handle transforms common to the three shifts, when the shift amount is a /// constant. +/// We are looking for: (shift being one of shl/sra/srl) +/// shift (binop X, C0), C1 +/// And want to transform into: +/// binop (shift X, C1), (shift C0, C1) SDValue DAGCombiner::visitShiftByConstant(SDNode *N, ConstantSDNode *Amt) { // Do not turn a 'not' into a regular xor. if (isBitwiseNot(N->getOperand(0))) return SDValue(); + // The inner binop must be one-use, since we want to replace it. SDNode *LHS = N->getOperand(0).getNode(); if (!LHS->hasOneUse()) return SDValue(); @@ -6594,27 +6599,23 @@ SDValue DAGCombiner::visitShiftByConstant(SDNode *N, ConstantSDNode *Amt) { // instead of (shift (and)), likewise for add, or, xor, etc. This sort of // thing happens with address calculations, so it's important to canonicalize // it. - bool HighBitSet = false; // Can we transform this if the high bit is set? - switch (LHS->getOpcode()) { - default: return SDValue(); + default: + return SDValue(); case ISD::OR: case ISD::XOR: - HighBitSet = false; // We can only transform sra if the high bit is clear. - break; case ISD::AND: - HighBitSet = true; // We can only transform sra if the high bit is set. break; case ISD::ADD: if (N->getOpcode() != ISD::SHL) return SDValue(); // only shl(add) not sr[al](add). - HighBitSet = false; // We can only transform sra if the high bit is clear. break; } // We require the RHS of the binop to be a constant and not opaque as well. ConstantSDNode *BinOpCst = getAsNonOpaqueConstant(LHS->getOperand(1)); - if (!BinOpCst) return SDValue(); + if (!BinOpCst) + return SDValue(); // FIXME: disable this unless the input to the binop is a shift by a constant // or is copy/select.Enable this in other cases when figure out it's exactly profitable. @@ -6634,16 +6635,6 @@ SDValue DAGCombiner::visitShiftByConstant(SDNode *N, ConstantSDNode *Amt) { EVT VT = N->getValueType(0); - // If this is a signed shift right, and the high bit is modified by the - // logical operation, do not perform the transformation. The highBitSet - // boolean indicates the value of the high bit of the constant which would - // cause it to be modified for this operation. - if (N->getOpcode() == ISD::SRA) { - bool BinOpRHSSignSet = BinOpCst->getAPIntValue().isNegative(); - if (BinOpRHSSignSet != HighBitSet) - return SDValue(); - } - if (!TLI.isDesirableToCommuteWithShift(N, Level)) return SDValue(); diff --git a/test/CodeGen/AArch64/pull-binop-through-shift.ll b/test/CodeGen/AArch64/pull-binop-through-shift.ll index 0dd93b2ce4d..6fee5984c8c 100644 --- a/test/CodeGen/AArch64/pull-binop-through-shift.ll +++ b/test/CodeGen/AArch64/pull-binop-through-shift.ll @@ -236,8 +236,8 @@ define i32 @and_nosignbit_ashr(i32 %x, i32* %dst) { define i32 @or_signbit_ashr(i32 %x, i32* %dst) { ; CHECK-LABEL: or_signbit_ashr: ; CHECK: // %bb.0: -; CHECK-NEXT: orr w8, w0, #0xffff0000 -; CHECK-NEXT: asr w0, w8, #8 +; CHECK-NEXT: lsr w8, w0, #8 +; CHECK-NEXT: orr w0, w8, #0xffffff00 ; CHECK-NEXT: str w0, [x1] ; CHECK-NEXT: ret %t0 = or i32 %x, 4294901760 ; 0xFFFF0000 @@ -261,8 +261,8 @@ define i32 @or_nosignbit_ashr(i32 %x, i32* %dst) { define i32 @xor_signbit_ashr(i32 %x, i32* %dst) { ; CHECK-LABEL: xor_signbit_ashr: ; CHECK: // %bb.0: -; CHECK-NEXT: eor w8, w0, #0xffff0000 -; CHECK-NEXT: asr w0, w8, #8 +; CHECK-NEXT: asr w8, w0, #8 +; CHECK-NEXT: eor w0, w8, #0xffffff00 ; CHECK-NEXT: str w0, [x1] ; CHECK-NEXT: ret %t0 = xor i32 %x, 4294901760 ; 0xFFFF0000 diff --git a/test/CodeGen/X86/pull-binop-through-shift.ll b/test/CodeGen/X86/pull-binop-through-shift.ll index a2086ddc0df..d4703d894bb 100644 --- a/test/CodeGen/X86/pull-binop-through-shift.ll +++ b/test/CodeGen/X86/pull-binop-through-shift.ll @@ -414,8 +414,8 @@ define i32 @or_signbit_ashr(i32 %x, i32* %dst) { ; X64-LABEL: or_signbit_ashr: ; X64: # %bb.0: ; X64-NEXT: movl %edi, %eax -; X64-NEXT: orl $-65536, %eax # imm = 0xFFFF0000 -; X64-NEXT: sarl $8, %eax +; X64-NEXT: shrl $8, %eax +; X64-NEXT: orl $-256, %eax ; X64-NEXT: movl %eax, (%rsi) ; X64-NEXT: retq ; @@ -459,8 +459,8 @@ define i32 @xor_signbit_ashr(i32 %x, i32* %dst) { ; X64-LABEL: xor_signbit_ashr: ; X64: # %bb.0: ; X64-NEXT: movl %edi, %eax -; X64-NEXT: xorl $-65536, %eax # imm = 0xFFFF0000 ; X64-NEXT: sarl $8, %eax +; X64-NEXT: xorl $-256, %eax ; X64-NEXT: movl %eax, (%rsi) ; X64-NEXT: retq ; diff --git a/test/CodeGen/X86/xor.ll b/test/CodeGen/X86/xor.ll index ba1a3df51cb..c2ec409058b 100644 --- a/test/CodeGen/X86/xor.ll +++ b/test/CodeGen/X86/xor.ll @@ -480,16 +480,16 @@ define %struct.ref_s* @test12(%struct.ref_s* %op, i64 %osbot, i64 %intval) { ; ; X64-LIN-LABEL: test12: ; X64-LIN: # %bb.0: -; X64-LIN-NEXT: notl %edx ; X64-LIN-NEXT: movslq %edx, %rax +; X64-LIN-NEXT: notq %rax ; X64-LIN-NEXT: shlq $4, %rax ; X64-LIN-NEXT: addq %rdi, %rax ; X64-LIN-NEXT: retq ; ; X64-WIN-LABEL: test12: ; X64-WIN: # %bb.0: -; X64-WIN-NEXT: notl %r8d ; X64-WIN-NEXT: movslq %r8d, %rax +; X64-WIN-NEXT: notq %rax ; X64-WIN-NEXT: shlq $4, %rax ; X64-WIN-NEXT: addq %rcx, %rax ; X64-WIN-NEXT: retq -- 2.50.1