[X86] Improve use of SHLD/SHRD

[deadalnix]

Summary

This extends the variety of pattern that can generate a SHLD instead of using two shifts.

This fixes a regression that would be introduced by D57367 or D33587

[RKSimon]

lib/Target/X86/X86ISelLowering.cpp

37054

Maybe use DAG.GetDemandedBits ?

[deadalnix]

lib/Target/X86/X86ISelLowering.cpp

37020

The operation is similar to what's done here.

37054

It's not obvious to me that it would work. The and opcode itself uses it, so there is no need to redo it here. We also need to know if masking took place to know what we can accept as LHS for the sub opcode.

[craig.topper]

lib/Target/X86/X86ISelLowering.cpp

37054

I know this code (Bits - 1) check is used in other places earlier in this function, but is that valid for i16 SHLD/SHRD? SHLD/SHRD hardware mask shift amount to 5 bits on i16/i32 and 6 bits on i64.

[RKSimon]

We might be better off just working on getting this code moved into DAGCombine (https://bugs.llvm.org/show_bug.cgi?id=40081) and use generic funnel shifts.

[deadalnix]

@RKSimon That sound reasonable. The main motivation for this patch is to fix a regression introduced by D57367 , so would that be possible to get at least a concept ack on it ?

lib/Target/X86/X86ISelLowering.cpp

37054

I do not know if this is valid for i16. If that isn't, this code is bogous already. Would you have a test case I can rely on to do this ?

[craig.topper]

lib/Target/X86/X86ISelLowering.cpp

37054

@RKSimon what are you thoughts on the existing uses of (Bits - 1) in this function for the i16 case? Those seem wrong to me.

[RKSimon]

lib/Target/X86/X86ISelLowering.cpp

37054

Yes I think its wrong - we're most likely being saved by the fact that its so tricky to create i16 double shifts from code (PR35155). As I said I'd much prefer to kill all this code and move it to FSHL/FSHR in DAGCombine (PR40081) - I'll take a look.

[RKSimon]

lib/Target/X86/X86ISelLowering.cpp

37054

Unravelling all of this code is proving trickier than I hoped due to various custom/legalization issues. I've committed rL353626 which folds to FSHL/FSHR and a fix for the i16 issue.

[RKSimon]

@deadalnix Please can you rebase this?

[deadalnix]

rebase on top of rL353626

[RKSimon]

LGTM

[deadalnix]

Rebase and add test cases taht do not depend on D57367

lib/Target/X86/X86ISelLowering.cpp

     ShAmt0 = ShAmt0.getOperand(0);
   }
   SDValue ShMsk1;
   if (ShAmt1.getOpcode() == ISD::AND &&
       isa<ConstantSDNode>(ShAmt1.getOperand(1)) &&
       ShAmt1.getConstantOperandVal(1) == (Bits - 1)) {
     ShMsk1 = ShAmt1;
     ShAmt1 = ShAmt1.getOperand(0);
   }
 
   if (ShAmt0.getOpcode() == ISD::TRUNCATE)
     ShAmt0 = ShAmt0.getOperand(0);
   if (ShAmt1.getOpcode() == ISD::TRUNCATE)
     ShAmt1 = ShAmt1.getOperand(0);
 
   SDLoc DL(N);
   unsigned Opc = X86ISD::SHLD;

   // OR( SHL( X, C ), SRL( SRL( Y, 1 ), XOR( C, 31 ) ) ) -> SHLD( X, Y, C )
   // OR( SRL( X, C ), SHL( SHL( Y, 1 ), XOR( C, 31 ) ) ) -> SHRD( X, Y, C )
   // OR( SHL( X, AND( C, 31 ) ), SRL( Y, AND( 0 - C, 31 ) ) ) -> SHLD( X, Y, C )
   // OR( SRL( X, AND( C, 31 ) ), SHL( Y, AND( 0 - C, 31 ) ) ) -> SHRD( X, Y, C )
   if (ShAmt1.getOpcode() == ISD::SUB) {
     SDValue Sum = ShAmt1.getOperand(0);
     if (auto *SumC = dyn_cast<ConstantSDNode>(Sum)) {
       SDValue ShAmt1Op1 = ShAmt1.getOperand(1);
+      if (ShAmt1Op1.getOpcode() == ISD::AND &&
+          isa<ConstantSDNode>(ShAmt1Op1.getOperand(1)) &&
+          ShAmt1Op1.getConstantOperandVal(1) == (Bits - 1)) {
+        ShMsk1 = ShAmt1Op1;
+        ShAmt1Op1 = ShAmt1Op1.getOperand(0);
+      }
       if (ShAmt1Op1.getOpcode() == ISD::TRUNCATE)
         ShAmt1Op1 = ShAmt1Op1.getOperand(0);
       if ((SumC->getAPIntValue() == Bits ||
            (SumC->getAPIntValue() == 0 && ShMsk1)) &&
           ShAmt1Op1 == ShAmt0)
         return DAG.getNode(Opc, DL, VT, Op0, Op1,
                            DAG.getNode(ISD::TRUNCATE, DL, MVT::i8, ShAmt0));
     }

test/CodeGen/X86/shift-double-x86_64.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-unknown | FileCheck %s
 
 ; SHLD/SHRD manual shifts
 
 define i64 @test1(i64 %hi, i64 %lo, i64 %bits) nounwind {
 ; CHECK-LABEL: test1:
 ; CHECK:       # %bb.0:
+; CHECK-NEXT:    movq %rdx, %rcx
 ; CHECK-NEXT:    movq %rdi, %rax
-; CHECK-NEXT:    movl %edx, %ecx
-; CHECK-NEXT:    andb $63, %cl
-; CHECK-NEXT:    negb %cl
-; CHECK-NEXT:    shrq %cl, %rsi
-; CHECK-NEXT:    movl %edx, %ecx
-; CHECK-NEXT:    shlq %cl, %rax
-; CHECK-NEXT:    orq %rsi, %rax
+; CHECK-NEXT:    # kill: def $cl killed $cl killed $rcx
+; CHECK-NEXT:    shldq %cl, %rsi, %rax
 ; CHECK-NEXT:    retq
   %and = and i64 %bits, 63
   %and64 = sub i64 64, %and
   %sh_lo = lshr i64 %lo, %and64
   %sh_hi = shl i64 %hi, %and
   %sh = or i64 %sh_lo, %sh_hi
   ret i64 %sh
 }
 
 define i64 @test2(i64 %hi, i64 %lo, i64 %bits) nounwind {
 ; CHECK-LABEL: test2:
 ; CHECK:       # %bb.0:
+; CHECK-NEXT:    movq %rdx, %rcx
 ; CHECK-NEXT:    movq %rsi, %rax
-; CHECK-NEXT:    movl %edx, %ecx
-; CHECK-NEXT:    andb $63, %cl
-; CHECK-NEXT:    negb %cl
-; CHECK-NEXT:    shlq %cl, %rdi
-; CHECK-NEXT:    movl %edx, %ecx
-; CHECK-NEXT:    shrq %cl, %rax
-; CHECK-NEXT:    orq %rdi, %rax
+; CHECK-NEXT:    # kill: def $cl killed $cl killed $rcx
+; CHECK-NEXT:    shrdq %cl, %rdi, %rax
 ; CHECK-NEXT:    retq
   %and = and i64 %bits, 63
   %and64 = sub i64 64, %and
   %sh_lo = shl i64 %hi, %and64
   %sh_hi = lshr i64 %lo, %and
   %sh = or i64 %sh_lo, %sh_hi
   ret i64 %sh
 }

test/CodeGen/X86/shift-double.ll

; X64-NEXT:    retq
   ret i64 %lshr
 }
 
 ; SHLD/SHRD manual shifts
 
 define i32 @test11(i32 %hi, i32 %lo, i32 %bits) nounwind {
 ; X86-LABEL: test11:
 ; X86:       # %bb.0:
-; X86-NEXT:    pushl %esi
+; X86-NEXT:    movb {{[0-9]+}}(%esp), %cl
+; X86-NEXT:    movl {{[0-9]+}}(%esp), %edx
 ; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; X86-NEXT:    movl {{[0-9]+}}(%esp), %esi
-; X86-NEXT:    movb {{[0-9]+}}(%esp), %dl
-; X86-NEXT:    movl %edx, %ecx
-; X86-NEXT:    andb $31, %cl
-; X86-NEXT:    negb %cl
-; X86-NEXT:    shrl %cl, %esi
-; X86-NEXT:    movl %edx, %ecx
-; X86-NEXT:    shll %cl, %eax
-; X86-NEXT:    orl %esi, %eax
-; X86-NEXT:    popl %esi
+; X86-NEXT:    shldl %cl, %edx, %eax
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: test11:
 ; X64:       # %bb.0:
-; X64-NEXT:    movl %edi, %eax
-; X64-NEXT:    movl %edx, %ecx
-; X64-NEXT:    andb $31, %cl
-; X64-NEXT:    negb %cl
-; X64-NEXT:    shrl %cl, %esi
 ; X64-NEXT:    movl %edx, %ecx
-; X64-NEXT:    shll %cl, %eax
-; X64-NEXT:    orl %esi, %eax
+; X64-NEXT:    movl %edi, %eax
+; X64-NEXT:    # kill: def $cl killed $cl killed $ecx
+; X64-NEXT:    shldl %cl, %esi, %eax
 ; X64-NEXT:    retq
   %and = and i32 %bits, 31
   %and32 = sub i32 32, %and
   %sh_lo = lshr i32 %lo, %and32
   %sh_hi = shl i32 %hi, %and
   %sh = or i32 %sh_lo, %sh_hi
   ret i32 %sh
 }
 
 define i32 @test12(i32 %hi, i32 %lo, i32 %bits) nounwind {
 ; X86-LABEL: test12:
 ; X86:       # %bb.0:
-; X86-NEXT:    pushl %esi
+; X86-NEXT:    movb {{[0-9]+}}(%esp), %cl
+; X86-NEXT:    movl {{[0-9]+}}(%esp), %edx
 ; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; X86-NEXT:    movl {{[0-9]+}}(%esp), %esi
-; X86-NEXT:    movb {{[0-9]+}}(%esp), %dl
-; X86-NEXT:    movl %edx, %ecx
-; X86-NEXT:    andb $31, %cl
-; X86-NEXT:    negb %cl
-; X86-NEXT:    shll %cl, %esi
-; X86-NEXT:    movl %edx, %ecx
-; X86-NEXT:    shrl %cl, %eax
-; X86-NEXT:    orl %esi, %eax
-; X86-NEXT:    popl %esi
+; X86-NEXT:    shrdl %cl, %edx, %eax
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: test12:
 ; X64:       # %bb.0:
-; X64-NEXT:    movl %esi, %eax
-; X64-NEXT:    movl %edx, %ecx
-; X64-NEXT:    andb $31, %cl
-; X64-NEXT:    negb %cl
-; X64-NEXT:    shll %cl, %edi
 ; X64-NEXT:    movl %edx, %ecx
-; X64-NEXT:    shrl %cl, %eax
-; X64-NEXT:    orl %edi, %eax
+; X64-NEXT:    movl %esi, %eax
+; X64-NEXT:    # kill: def $cl killed $cl killed $ecx
+; X64-NEXT:    shrdl %cl, %edi, %eax
 ; X64-NEXT:    retq
   %and = and i32 %bits, 31
   %and32 = sub i32 32, %and
   %sh_lo = shl i32 %hi, %and32
   %sh_hi = lshr i32 %lo, %and
   %sh = or i32 %sh_lo, %sh_hi
   ret i32 %sh
 }