[DAGCombiner] visitShiftByConstant(): drop bogus(?) signbit check

[lebedev.ri]

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
4. 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?

[lebedev.ri]

(adding reviewers from D27916)

[craig.topper]

This logic was copied from canShiftBinOpWithConstantRHS in InstCombineShifts.cpp

[lebedev.ri]

In D61918#1502313, @craig.topper wrote:

This logic was copied from canShiftBinOpWithConstantRHS in InstCombineShifts.cpp

Aha, that makes sense. Posted D61938.

[spatel]

test/CodeGen/X86/xor.ll

496–498 ↗

(On Diff #199516)

This test (and the 1 below here) wasn't resulting in 'not' asm before rL347478, so I was trying to make that happen.

This diff seems like a (slight) regression because we're turning a 32-bit 'not' into a 64-bit 'not' (extra prefix byte?), but I'm not sure if anyone cares.

[lebedev.ri]

Now with tests.

[lebedev.ri]

In D61918#1502732, @lebedev.ri wrote:

In D61918#1502313, @craig.topper wrote:

This logic was copied from canShiftBinOpWithConstantRHS in InstCombineShifts.cpp

Aha, that makes sense. Posted D61938.

Does this look good now that this fix for the origin of this code is accepted?

[spatel]

LGTM - might want to leave a 'TODO' comment for the x86 test that can use the narrower 'not' instruction.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitXOR(SDNode *N) {
   if (SimplifyDemandedBits(SDValue(N, 0)))
     return SDValue(N, 0);
 
   return SDValue();
 }
 
 /// 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();
 
   // We want to pull some binops through shifts, so that we have (and (shift))
   // 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.
   SDNode *BinOpLHSVal = LHS->getOperand(0).getNode();
   bool isShift = BinOpLHSVal->getOpcode() == ISD::SHL ||
                  BinOpLHSVal->getOpcode() == ISD::SRA ||
                  BinOpLHSVal->getOpcode() == ISD::SRL;
   bool isCopyOrSelect = BinOpLHSVal->getOpcode() == ISD::CopyFromReg ||
                         BinOpLHSVal->getOpcode() == ISD::SELECT;
 
   if ((!isShift || !isa<ConstantSDNode>(BinOpLHSVal->getOperand(1))) &&
       !isCopyOrSelect)
     return SDValue();
 
   if (isCopyOrSelect && N->hasOneUse())
     return SDValue();
 
   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();
 
   // Fold the constants, shifting the binop RHS by the shift amount.
   SDValue NewRHS = DAG.getNode(N->getOpcode(), SDLoc(LHS->getOperand(1)),
                                N->getValueType(0),
                                LHS->getOperand(1), N->getOperand(1));
   assert(isa<ConstantSDNode>(NewRHS) && "Folding was not successful!");

llvm/trunk/test/CodeGen/AArch64/pull-binop-through-shift.ll

; CHECK-NEXT:    ret
   %r = ashr i32 %t0, 8
   store i32 %r, i32* %dst
   ret i32 %r
 }
 
 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
   %r = ashr i32 %t0, 8
   store i32 %r, i32* %dst
   ret i32 %r
 }
 define i32 @or_nosignbit_ashr(i32 %x, i32* %dst) {
 ; CHECK-LABEL: or_nosignbit_ashr:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    asr w8, w0, #8
 ; CHECK-NEXT:    orr w0, w8, #0x7fff00
 ; CHECK-NEXT:    str w0, [x1]
 ; CHECK-NEXT:    ret
   %t0 = or i32 %x, 2147418112 ; 0x7FFF0000
   %r = ashr i32 %t0, 8
   store i32 %r, i32* %dst
   ret i32 %r
 }
 
 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
   %r = ashr i32 %t0, 8
   store i32 %r, i32* %dst
   ret i32 %r
 }
 define i32 @xor_nosignbit_ashr(i32 %x, i32* %dst) {

llvm/trunk/test/CodeGen/X86/pull-binop-through-shift.ll

; X32-NEXT:    retl
   store i32 %r, i32* %dst
   ret i32 %r
 }
 
 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
 ;
 ; X32-LABEL: or_signbit_ashr:
 ; X32:       # %bb.0:
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %ecx
 ; X32-NEXT:    movl $-65536, %eax # imm = 0xFFFF0000
 ; X32-NEXT:    orl {{[0-9]+}}(%esp), %eax
; X32-NEXT:    retl
   store i32 %r, i32* %dst
   ret i32 %r
 }
 
 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
 ;
 ; X32-LABEL: xor_signbit_ashr:
 ; X32:       # %bb.0:
 ; X32-NEXT:    movl {{[0-9]+}}(%esp), %ecx
 ; X32-NEXT:    movl $-65536, %eax # imm = 0xFFFF0000
 ; X32-NEXT:    xorl {{[0-9]+}}(%esp), %eax

llvm/trunk/test/CodeGen/X86/xor.ll

 ; X32-NEXT:    notl %eax
 ; X32-NEXT:    leal (%eax,%eax,2), %eax
 ; X32-NEXT:    shll $2, %eax
 ; X32-NEXT:    addl {{[0-9]+}}(%esp), %eax
 ; X32-NEXT:    retl
 ;
 ; 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
   %neg = shl i64 %intval, 32
   %sext = xor i64 %neg, -4294967296
   %idx.ext = ashr exact i64 %sext, 32
   %add.ptr = getelementptr inbounds %struct.ref_s, %struct.ref_s* %op, i64 %idx.ext
   ret %struct.ref_s* %add.ptr