PR31357 fix

[dtemirbulatov]

Summary

This patch was introduced by Jim Lewis but it have not been reviewed properly yet, OK to commit?

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3887 ↗

(On Diff #95184)

Remove braces (style)

3891 ↗

(On Diff #95184)

Move this comment before the first if() - it seems to be common to both cases.

test/CodeGen/X86/bswap_tree.ll

4 ↗

(On Diff #95184)

Regenerate the codegen with llvm\utils\update_llc_test_checks.py

Also, lease use triples instead of arch/cpu:

; RUN: llc < %s -mtriple=i686-unknown | FileCheck %s
; RUN: llc < %s -mtriple=x86_64-unknown | FileCheck %s --check-prefix=CHECK64

test/CodeGen/X86/bswap_tree2.ll

5 ↗

(On Diff #95184)

Regenerate the codegen with llvm\utils\update_llc_test_checks.py

Also, lease use triples instead of arch/cpu:

; RUN: llc < %s -mtriple=i686-unknown | FileCheck %s
; RUN: llc < %s -mtriple=x86_64-unknown | FileCheck %s --check-prefix=CHECK64

[dtemirbulatov]

update after Simon's comments.

[dtemirbulatov]

updated the change after rebase.

[dtemirbulatov]

update again, something went wrong during the last update.

[dtemirbulatov]

added update to tests.

[dtemirbulatov]

update test again with utils/update_llc_test_checks.py

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3895 ↗

(On Diff #95773)

Style guide says variables should be Capitalized. It might be clearer to call it MaskByteOffset or similar. If possible this should be done as a separate NFC commit.

[dtemirbulatov]

update after Simon's comments.

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3895 ↗

(On Diff #95773)

+1 to making this "MaskByteOffset" and doing that now as NFC, so this patch will be minimized. Also, make the change for Opc0 = N0.getOpcode() ahead of this patch. That should leave us with the actual functional changes.

[filcab]

I'd rather see this as 3 commits:

• s/TLI.isOperationLegal/TLI.isOperationLegalOrCustom/
• Refactor (name changes)
• Actual fix for PR31357

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3769 ↗

(On Diff #96350)

This looks like a drive-by fix, can you split it?

3962 ↗

(On Diff #96350)

Same as above.

[dtemirbulatov]

Some variable name changes.

[dtemirbulatov]

Further changes after rebase.

[RKSimon]

LGTM

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

 SDValue DAGCombiner::MatchBSwapHWordLow(SDNode *N, SDValue N0, SDValue N1,
                                         bool DemandHighBits) {
   if (!LegalOperations)
     return SDValue();
 
   EVT VT = N->getValueType(0);
   if (VT != MVT::i64 && VT != MVT::i32 && VT != MVT::i16)
     return SDValue();
-  if (!TLI.isOperationLegal(ISD::BSWAP, VT))
+  if (!TLI.isOperationLegalOrCustom(ISD::BSWAP, VT))
     return SDValue();
 
   // Recognize (and (shl a, 8), 0xff), (and (srl a, 8), 0xff00)
   bool LookPassAnd0 = false;
   bool LookPassAnd1 = false;
   if (N0.getOpcode() == ISD::AND && N0.getOperand(0).getOpcode() == ISD::SRL)
       std::swap(N0, N1);
   if (N1.getOpcode() == ISD::AND && N1.getOperand(0).getOpcode() == ISD::SHL)
if (!N.getNode()->hasOneUse())
     return false;
 
   unsigned Opc = N.getOpcode();
   if (Opc != ISD::AND && Opc != ISD::SHL && Opc != ISD::SRL)
     return false;
 
   SDValue N0 = N.getOperand(0);
   unsigned Opc0 = N0.getOpcode();
+  if (Opc0 != ISD::AND && Opc0 != ISD::SHL && Opc0 != ISD::SRL)
+    return false;
 
-  ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N.getOperand(1));
+  ConstantSDNode *N1C = nullptr;
+  // SHL or SRL: look upstream for AND mask operand
+  if (Opc == ISD::AND)
+    N1C = dyn_cast<ConstantSDNode>(N.getOperand(1));
+  else if (Opc0 == ISD::AND)
+    N1C = dyn_cast<ConstantSDNode>(N0.getOperand(1));
   if (!N1C)
     return false;
 
   unsigned MaskByteOffset;
   switch (N1C->getZExtValue()) {
   default:
     return false;
   case 0xFF:       MaskByteOffset = 0; break;

 /// => (rotl (bswap x), 16)
 SDValue DAGCombiner::MatchBSwapHWord(SDNode *N, SDValue N0, SDValue N1) {
   if (!LegalOperations)
     return SDValue();
 
   EVT VT = N->getValueType(0);
   if (VT != MVT::i32)
     return SDValue();
-  if (!TLI.isOperationLegal(ISD::BSWAP, VT))
+  if (!TLI.isOperationLegalOrCustom(ISD::BSWAP, VT))
     return SDValue();
 
   // Look for either
   // (or (or (and), (and)), (or (and), (and)))
   // (or (or (or (and), (and)), (and)), (and))
   if (N0.getOpcode() != ISD::OR)
     return SDValue();
   SDValue N00 = N0.getOperand(0);
   SDValue N01 = N0.getOperand(1);
   SDNode *Parts[4] = {};
 
   if (N1.getOpcode() == ISD::OR &&
       N00.getNumOperands() == 2 && N01.getNumOperands() == 2) {
     // (or (or (and), (and)), (or (and), (and)))
-    SDValue N000 = N00.getOperand(0);
-    if (!isBSwapHWordElement(N000, Parts))
+    if (!isBSwapHWordElement(N00, Parts))
       return SDValue();
 
-    SDValue N001 = N00.getOperand(1);
-    if (!isBSwapHWordElement(N001, Parts))
+    if (!isBSwapHWordElement(N01, Parts))
       return SDValue();
-    SDValue N010 = N01.getOperand(0);
-    if (!isBSwapHWordElement(N010, Parts))
+    SDValue N10 = N1.getOperand(0);
+    if (!isBSwapHWordElement(N10, Parts))
       return SDValue();
-    SDValue N011 = N01.getOperand(1);
-    if (!isBSwapHWordElement(N011, Parts))
+    SDValue N11 = N1.getOperand(1);
+    if (!isBSwapHWordElement(N11, Parts))
       return SDValue();
   } else {
     // (or (or (or (and), (and)), (and)), (and))
     if (!isBSwapHWordElement(N1, Parts))
       return SDValue();
     if (!isBSwapHWordElement(N01, Parts))
       return SDValue();
     if (N00.getOpcode() != ISD::OR)

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

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-unknown-unknown | FileCheck %s
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s --check-prefix=CHECK64
 
 ; Check reconstructing bswap from shifted masks and tree of ORs
 
 ; Match a 32-bit packed halfword bswap. That is
 ; ((x & 0x000000ff) << 8) |
 ; ((x & 0x0000ff00) >> 8) |
 ; ((x & 0x00ff0000) << 8) |
 ; ((x & 0xff000000) >> 8)
 ; => (rotl (bswap x), 16)
 define i32 @test1(i32 %x) nounwind {
 ; CHECK-LABEL: test1:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %ecx
-; CHECK-NEXT:    movl %ecx, %edx
-; CHECK-NEXT:    andl $16711680, %edx # imm = 0xFF0000
-; CHECK-NEXT:    movl %ecx, %eax
-; CHECK-NEXT:    andl $-16777216, %eax # imm = 0xFF000000
-; CHECK-NEXT:    shll $8, %edx
-; CHECK-NEXT:    shrl $8, %eax
-; CHECK-NEXT:    bswapl %ecx
-; CHECK-NEXT:    shrl $16, %ecx
-; CHECK-NEXT:    orl %edx, %eax
-; CHECK-NEXT:    orl %ecx, %eax
+; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; CHECK-NEXT:    bswapl %eax
+; CHECK-NEXT:    roll $16, %eax
 ; CHECK-NEXT:    retl
 ;
 ; CHECK64-LABEL: test1:
 ; CHECK64:       # BB#0:
-; CHECK64-NEXT:    # kill: %EDI<def> %EDI<kill> %RDI<def>
-; CHECK64-NEXT:    movl %edi, %eax
-; CHECK64-NEXT:    andl $16711680, %eax # imm = 0xFF0000
-; CHECK64-NEXT:    movl %edi, %ecx
-; CHECK64-NEXT:    andl $-16777216, %ecx # imm = 0xFF000000
-; CHECK64-NEXT:    shll $8, %eax
-; CHECK64-NEXT:    shrl $8, %ecx
 ; CHECK64-NEXT:    bswapl %edi
-; CHECK64-NEXT:    shrl $16, %edi
-; CHECK64-NEXT:    orl %eax, %ecx
-; CHECK64-NEXT:    leal (%rcx,%rdi), %eax
+; CHECK64-NEXT:    roll $16, %edi
+; CHECK64-NEXT:    movl %edi, %eax
 ; CHECK64-NEXT:    retq
   %byte0 = and i32 %x, 255        ; 0x000000ff
   %byte1 = and i32 %x, 65280      ; 0x0000ff00
   %byte2 = and i32 %x, 16711680   ; 0x00ff0000
   %byte3 = and i32 %x, 4278190080 ; 0xff000000
   %tmp0 = shl  i32 %byte0, 8
   %tmp1 = lshr i32 %byte1, 8
   %tmp2 = shl  i32 %byte2, 8
   %tmp3 = lshr i32 %byte3, 8
   %or0 = or i32 %tmp0, %tmp1
   %or1 = or i32 %tmp2, %tmp3
   %result = or i32 %or0, %or1
   ret i32 %result
 }
 
 ; the same as test1, just shifts before the "and"
 ; ((x << 8) & 0x0000ff00) |
 ; ((x >> 8) & 0x000000ff) |
 ; ((x << 8) & 0xff000000) |
 ; ((x >> 8) & 0x00ff0000)
 define i32 @test2(i32 %x) nounwind {
 ; CHECK-LABEL: test2:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    pushl %esi
 ; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK-NEXT:    movl %eax, %ecx
-; CHECK-NEXT:    shll $8, %ecx
-; CHECK-NEXT:    shrl $8, %eax
-; CHECK-NEXT:    movzwl %cx, %edx
-; CHECK-NEXT:    movzbl %al, %esi
-; CHECK-NEXT:    andl $-16777216, %ecx # imm = 0xFF000000
-; CHECK-NEXT:    andl $16711680, %eax # imm = 0xFF0000
-; CHECK-NEXT:    orl %edx, %esi
-; CHECK-NEXT:    orl %ecx, %eax
-; CHECK-NEXT:    orl %esi, %eax
-; CHECK-NEXT:    popl %esi
+; CHECK-NEXT:    bswapl %eax
+; CHECK-NEXT:    roll $16, %eax
 ; CHECK-NEXT:    retl
 ;
 ; CHECK64-LABEL: test2:
 ; CHECK64:       # BB#0:
-; CHECK64-NEXT:    movl %edi, %ecx
-; CHECK64-NEXT:    shll $8, %ecx
-; CHECK64-NEXT:    shrl $8, %edi
-; CHECK64-NEXT:    movzwl %cx, %edx
-; CHECK64-NEXT:    movzbl %dil, %eax
-; CHECK64-NEXT:    andl $-16777216, %ecx # imm = 0xFF000000
-; CHECK64-NEXT:    andl $16711680, %edi # imm = 0xFF0000
-; CHECK64-NEXT:    orl %edx, %eax
-; CHECK64-NEXT:    orl %ecx, %edi
-; CHECK64-NEXT:    orl %edi, %eax
+; CHECK64-NEXT:    bswapl %edi
+; CHECK64-NEXT:    roll $16, %edi
+; CHECK64-NEXT:    movl %edi, %eax
 ; CHECK64-NEXT:    retq
   %byte1 = shl  i32 %x, 8
   %byte0 = lshr i32 %x, 8
   %byte3 = shl  i32 %x, 8
   %byte2 = lshr i32 %x, 8
   %tmp1 = and i32 %byte1, 65280      ; 0x0000ff00
   %tmp0 = and i32 %byte0, 255        ; 0x000000ff
   %tmp3 = and i32 %byte3, 4278190080 ; 0xff000000
   %tmp2 = and i32 %byte2, 16711680   ; 0x00ff0000
   %or0 = or i32 %tmp0, %tmp1
   %or1 = or i32 %tmp2, %tmp3
   %result = or i32 %or0, %or1
   ret i32 %result
 }