[X86][MMX] Added custom lowering action for MMX SELECT (PR30418)

[kbelochapka]

Summary

Fix for pr30418 - error in backend: Cannot select: t17: x86mmx = select_cc t2, Constant:i64<0>, t7, t8, seteq:ch

[RKSimon]

Thanks for looking at this!

Very minor, but can you make sure that the patch title is something like "[X86][MMX] Added custom lowering action for MMX SELECT (PR30418)" and then make your current title the summary - it makes finding reviews/commits easier.

test/CodeGen/X86/pr30418.ll

5 ↗

(On Diff #104054)

If possible, we need better test coverage here. Please can you rename the file select-mmx.ll, rename this test pr30418 and add filecheck and update_llc_test_checks.py codegen generation.

You also need to test on a i686 triple as well since you're creating i64 values.

A few extra tests wouldn't go amiss - select with non-constant mmx values for instance.

[kbelochapka]

fix now works for 32bit mode, the test updated from crash checking to positive

[RKSimon]

This diff looks like it has gone bad being rebased?

test/CodeGen/X86/select-mmx.ll

3

Use triples + attributes instead of arch/cpu (avoids test failure on different machines):

; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+mmx < %s | FileCheck %s --check-prefix=X64
; RUN: llc -mtriple=i686-unknown-unknown -mattr=+mmx < %s | FileCheck %s --check-prefix=I32

[kbelochapka]

replaced llc option -mcpu for -mtriple, the diff file reformatted

[RKSimon]

lib/Target/X86/X86ISelLowering.cpp

423

This diff is still broken - I'm assuming it should be before this for loop and after the loop with the other SELECT/SETCC declarations?

30951

Again, the diff is broken - its embedded in the middle of combineCMov's constant folding code.....

[kbelochapka]

updated poorly formated diff file

[RKSimon]

Code now looks fine, just some further tidying up of the test cases

test/CodeGen/X86/select-mmx.ll

2

You should regenerate the codegen with utils/update_llc_test_checks.py

12

You can drop 'local_unnamed_addr #1' and probably add 'nonuwind' instead

49

Remove the #3

62

You can drop 'local_unnamed_addr #1' and probably add 'nonuwind' instead

97

Remove the #3

102

Remove the #2

[kbelochapka]

test regenerated with update_llc_test_checks.py, unnecessary artifacts removed.

[RKSimon]

LGTM - thanks

lib/Target/X86/X86ISelLowering.cpp

for (auto VT : { MVT::f32, MVT::f64, MVT::f80, MVT::f128 }) {
     setOperationAction(ISD::SETCC, VT, Custom);
   }
   for (auto VT : { MVT::i8, MVT::i16, MVT::i32, MVT::i64 }) {
     if (VT == MVT::i64 && !Subtarget.is64Bit())
       continue;
     setOperationAction(ISD::SELECT, VT, Custom);
     setOperationAction(ISD::SETCC,  VT, Custom);
   }
+
+  // Custom action for SELECT MMX and expand action for SELECT_CC MMX
+  setOperationAction(ISD::SELECT, MVT::x86mmx, Custom);
+  setOperationAction(ISD::SELECT_CC, MVT::x86mmx, Expand);
+
   setOperationAction(ISD::EH_RETURN       , MVT::Other, Custom);
   // NOTE: EH_SJLJ_SETJMP/_LONGJMP supported here is NOT intended to support
   // SjLj exception handling but a light-weight setjmp/longjmp replacement to
   // support continuation, user-level threading, and etc.. As a result, no
   // other SjLj exception interfaces are implemented and please don't build
   // your own exception handling based on them.
   // LLVM/Clang supports zero-cost DWARF exception handling.
   setOperationAction(ISD::EH_SJLJ_SETJMP, MVT::i32, Custom);
static SDValue combineSelect(SDNode *N, SelectionDAG &DAG,
   if (N->getOpcode() == ISD::VSELECT && DCI.isAfterLegalizeVectorOps() &&
       CondVT.getVectorElementType() == MVT::i1) {
     if (combineBitcastForMaskedOp(LHS, DAG, DCI))
       return SDValue(N, 0);
     if (combineBitcastForMaskedOp(RHS, DAG, DCI))
       return SDValue(N, 0);
   }
 
+  // Custom action for SELECT MMX
+  if (VT == MVT::x86mmx) {
+    LHS = DAG.getBitcast(MVT::i64, LHS);
+    RHS = DAG.getBitcast(MVT::i64, RHS);
+    SDValue newSelect = DAG.getNode(ISD::SELECT, DL, MVT::i64, Cond, LHS, RHS);
+    return DAG.getBitcast(VT, newSelect);
+  }
+
   return SDValue();
 }
 
 /// Combine:
 ///   (brcond/cmov/setcc .., (cmp (atomic_load_add x, 1), 0), COND_S)
 /// to:
 ///   (brcond/cmov/setcc .., (LADD x, 1), COND_LE)
 /// i.e., reusing the EFLAGS produced by the LOCKed instruction.
if (ConstantSDNode *FalseC = dyn_cast<ConstantSDNode>(FalseOp)) {
 
         unsigned ShAmt = TrueC->getAPIntValue().logBase2();
         Cond = DAG.getNode(ISD::SHL, DL, Cond.getValueType(), Cond,
                            DAG.getConstant(ShAmt, DL, MVT::i8));
         if (N->getNumValues() == 2)  // Dead flag value?
           return DCI.CombineTo(N, Cond, SDValue());
         return Cond;
       }
 
       // Optimize Cond ? cst+1 : cst -> zext(setcc(C)+cst.  This is efficient
       // for any integer data type, including i8/i16.
       if (FalseC->getAPIntValue()+1 == TrueC->getAPIntValue()) {
         Cond = getSETCC(CC, Cond, DL, DAG);
 
         // Zero extend the condition if needed.
         Cond = DAG.getNode(ISD::ZERO_EXTEND, DL,
                            FalseC->getValueType(0), Cond);

test/CodeGen/X86/select-mmx.ll

+; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+mmx < %s | FileCheck %s --check-prefix=X64
+; RUN: llc -mtriple=i686-unknown-unknown -mattr=+mmx < %s | FileCheck %s --check-prefix=I32
+
+
+; From source: clang -02
+;__m64 test47(int a)
+;{
+;    __m64 x = (a)? (__m64)(7): (__m64)(0);
+; return __builtin_ia32_psllw(x, x);
+;}
+
+define i64 @test47(i64 %arg) local_unnamed_addr #1 {
+;
+; I32-LABEL:  test47:                                 # @test47
+; I32:      .cfi_startproc
+; I32-NEXT: # BB#0:
+; I32-NEXT:   subl  [[ST:[$][0-9]+]], [[ESP:%e[a-z][x|i|p]]]
+; I32-NEXT: .Lcfi0:
+; I32-NEXT:   .cfi_def_cfa_offset {{[0-9]+}}
+; I32-NEXT:   movl  {{[0-9]*}}([[ESP]]), [[EAX:%e[a-z][x|i]]]
+; I32-NEXT:   xorl  [[ECX:%e[a-z][x|i]]], [[ECX]]
+; I32-NEXT:   orl {{[0-9]*}}([[ESP]]), [[EAX]]
+; I32-NEXT:   movl  $7, [[EAX]]
+; I32-NEXT:   cmovnel [[ECX]], [[EAX]]
+; I32-NEXT:   movl  [[EAX]], {{[0-9]*}}([[ESP]])
+; I32-NEXT:   movl  $0, {{[0-9]*}}([[ESP]])
+; I32-NEXT:   movq  {{[0-9]+}}([[ESP]]), [[MMX0:%mm[0-9]]]
+; I32-NEXT:   psllw [[MMX0]], [[MMX0]]
+; I32-NEXT:   movq  [[MMX0]], {{[0-9]*}}([[ESP]])
+; I32-NEXT:   movl  {{[0-9]*}}([[ESP]]), [[EAX:%e[a-z][x|i]]]
+; I32-NEXT:   movl  {{[0-9]*}}([[ESP]]), [[EDX:%e[a-z][x|i]]]
+; I32-NEXT:   addl  [[ST]], [[ESP]]
+; I32-NEXT:   retl
+;
+; X64-LABEL:  test47:                                 # @test47
+; X64:      .cfi_startproc
+; X64-NEXT: # BB#0:
+; X64-NEXT:   xorl  [[EAX:%e[a-z][x|i]]], [[EAX:%e[a-z][x|i]]]
+; X64-NEXT:   testq [[RDI:%r[a-z][x|i]]], [[RDI]]
+; X64-NEXT:   movl  $7, [[ECX:%e[a-z][x|i]]]
+; X64-NEXT:   cmoveq  [[RCX:%r[a-z][x|i]]], [[RAX:%r[a-z][x|i]]]
+; X64-NEXT:   movd  [[RAX]], [[MMX0:%mm[0-9]]]
+; X64-NEXT:   psllw [[MMX0]], [[MMX0]]
+; X64-NEXT:   movd  [[MMX0]], [[RAX]]
+; X64-NEXT:   retq
+;
+  %cond = icmp eq i64 %arg, 0
+  %slct = select i1 %cond, x86_mmx bitcast (i64 7 to x86_mmx), x86_mmx bitcast (i64 0 to x86_mmx)
+  %psll = tail call x86_mmx @llvm.x86.mmx.psll.w(x86_mmx %slct, x86_mmx %slct) #3
+  %retc = bitcast x86_mmx %psll to i64
+  ret i64 %retc
+}
+
+
+; From source: clang -O2
+;__m64 test49(int a, long long n, long long m)
+;{
+;    __m64 x = (a)? (__m64)(n): (__m64)(m);
+; return __builtin_ia32_psllw(x, x);
+;}
+
+define i64 @test49(i64 %arg, i64 %x, i64 %y) local_unnamed_addr #1 {
+;
+; I32-LABEL:  test49:                                 # @test49
+; I32:      .cfi_startproc
+; I32-NEXT: # BB#0:
+; I32-NEXT:   subl  [[ST:[$][0-9]+]], [[ESP:%e[a-z][x|i|p]]]
+; I32-NEXT: .Lcfi1:
+; I32-NEXT:   .cfi_def_cfa_offset {{[0-9]+}}
+; I32-NEXT:   movl  {{[0-9]*}}([[ESP]]), [[EAX:%e[a-z][x|i]]]
+; I32-NEXT:   leal  {{[0-9]*}}([[ESP]]), [[ECX:%e[a-z][x|i]]]
+; I32-NEXT:   orl {{[0-9]*}}([[ESP]]), [[EAX]]
+; I32-NEXT:   leal  {{[0-9]*}}([[ESP]]), [[EAX]]
+; I32-NEXT:   cmovel  [[EAX]], [[ECX]]
+; I32-NEXT:   movq  ([[ECX]]), [[MMX0:%mm[0-9]]]
+; I32-NEXT:   psllw [[MMX0]], [[MMX0]]
+; I32-NEXT:   movq  [[MMX0]], {{[0-9]*}}([[ESP]])
+; I32-NEXT:   movl  {{[0-9]*}}([[ESP]]), [[EAX:%e[a-z][x|i]]]
+; I32-NEXT:   movl  {{[0-9]*}}([[ESP]]), [[EDX:%e[a-z][x|i]]]
+; I32-NEXT:   addl  [[ST]], [[ESP]]
+; I32-NEXT:   retl
+;
+; X64-LABEL:  test49:                                 # @test49
+; X64:      .cfi_startproc
+; X64-NEXT: # BB#0:
+; X64-NEXT:   testq [[RDI:%r[a-z][x|i]]], [[RDI]]
+; X64-NEXT:   cmovneq [[RDX:%r[a-z][x|i]]], [[RSI:%r[a-z][x|i]]]
+; X64-NEXT:   movd  [[RSI]], [[MMX0:%mm[0-9]]]
+; X64-NEXT:   psllw [[MMX0]], [[MMX0]]
+; X64-NEXT:   movd  [[MMX0]], [[RAX:%r[a-z][x|i]]]
+; X64-NEXT:   retq
+;
+  %cond = icmp eq i64 %arg, 0
+  %xmmx = bitcast i64 %x to x86_mmx
+  %ymmx = bitcast i64 %y to x86_mmx
+  %slct = select i1 %cond, x86_mmx %xmmx, x86_mmx %ymmx
+  %psll = tail call x86_mmx @llvm.x86.mmx.psll.w(x86_mmx %slct, x86_mmx %slct) #3
+  %retc = bitcast x86_mmx %psll to i64
+  ret i64 %retc
+}
+
+declare x86_mmx @llvm.x86.mmx.psll.w(x86_mmx, x86_mmx) #2
+