[AVX512][FastISel] Do not use K registers in TEST instructions

[guyblank]

Summary

since i1 is legal in AVX512, i1 values weren't promoted to i8, instead K registers were allocated.
this causes an issue when they are passed to a TEST instruction, which is illegal.

Solves Bug 28661

while this patch solves this specific issue, what I would really want to do is make the isel choose GPRs for these i1 values.

[delena]

Any test?

[delena]

lib/Target/X86/X86FastISel.cpp

1694 ↗

(On Diff #66799)

It may give incorrect result. If the OpReg is k-reg you can use k-shitft-left 15 with ktestw.

[guyblank]

as Elena suggested, changed the sequence to kshift left & ktest.
added a test.

[igorb]

lib/Target/X86/X86FastISel.cpp

1695 ↗

(On Diff #68013)

KTEST instruction require AVX512DQ, it will fail to run (illegal instruction) on knl target.
We assume that VK1 is always zero extended, you don't need shift.
I suggest to use KORTESTW instruction ( it require only AVX512F , available if VK1 legal ), in this case it have the same semantic.

test/CodeGen/X86/fast-isel-select-cmov.ll

1 ↗

(On Diff #68013)

could you please change it to
; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10 | FileCheck %s --check-prefix=CHECK --check-prefix=NOAVX512
; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10 -mattr=+avx512f | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512

and regenerate the checks using update_llc_test_checks utils.

[guyblank]

changes according to igor's comments

[igorb]

lib/Target/X86/X86FastISel.cpp

2039 ↗

(On Diff #68496)

could you please check if this one should be addReg(CondReg, getKillRegState(CondIsKill)) ?

2220 ↗

(On Diff #68496)

could you please check if this one should be addReg(CondReg, getKillRegState(CondIsKill))?

[guyblank]

add kill state

[igorb]

LGTM

llvm/trunk/lib/Target/X86/X86FastISel.cpp

if (TI->hasOneUse() && TI->getParent() == I->getParent() &&
       case MVT::i8:  TestOpc = X86::TEST8ri; break;
       case MVT::i16: TestOpc = X86::TEST16ri; break;
       case MVT::i32: TestOpc = X86::TEST32ri; break;
       case MVT::i64: TestOpc = X86::TEST64ri32; break;
       }
       if (TestOpc) {
         unsigned OpReg = getRegForValue(TI->getOperand(0));
         if (OpReg == 0) return false;
+
         BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(TestOpc))
           .addReg(OpReg).addImm(1);
 
         unsigned JmpOpc = X86::JNE_1;
         if (FuncInfo.MBB->isLayoutSuccessor(TrueMBB)) {
           std::swap(TrueMBB, FalseMBB);
           JmpOpc = X86::JE_1;
         }
bool X86FastISel::X86SelectBranch(const Instruction *I) {
   }
 
   // Otherwise do a clumsy setcc and re-test it.
   // Note that i1 essentially gets ANY_EXTEND'ed to i8 where it isn't used
   // in an explicit cast, so make sure to handle that correctly.
   unsigned OpReg = getRegForValue(BI->getCondition());
   if (OpReg == 0) return false;
 
+  // In case OpReg is a K register, kortest against itself.
+  if (MRI.getRegClass(OpReg) == &X86::VK1RegClass)
+    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(X86::KORTESTWrr))
+        .addReg(OpReg)
+        .addReg(OpReg);
+  else
     BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(X86::TEST8ri))
-    .addReg(OpReg).addImm(1);
+        .addReg(OpReg)
+        .addImm(1);
   BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(X86::JNE_1))
     .addMBB(TrueMBB);
   finishCondBranch(BI->getParent(), TrueMBB, FalseMBB);
   return true;
 }
 
 bool X86FastISel::X86SelectShift(const Instruction *I) {
   unsigned CReg = 0, OpReg = 0;
if (NeedTest) {
     // accurate. If we read more than the lsb, we may see non-zero values
     // whereas lsb is zero. Therefore, we have to truncate Op0Reg to i1 for
     // the select. This is achieved by performing TEST against 1.
     unsigned CondReg = getRegForValue(Cond);
     if (CondReg == 0)
       return false;
     bool CondIsKill = hasTrivialKill(Cond);
 
+    // In case OpReg is a K register, kortest against itself.
+    if (MRI.getRegClass(CondReg) == &X86::VK1RegClass)
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+              TII.get(X86::KORTESTWrr))
+          .addReg(CondReg, getKillRegState(CondIsKill))
+          .addReg(CondReg);
+    else
       BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(X86::TEST8ri))
-      .addReg(CondReg, getKillRegState(CondIsKill)).addImm(1);
+          .addReg(CondReg, getKillRegState(CondIsKill))
+          .addImm(1);
   }
 
   const Value *LHS = I->getOperand(1);
   const Value *RHS = I->getOperand(2);
 
   unsigned RHSReg = getRegForValue(RHS);
   bool RHSIsKill = hasTrivialKill(RHS);
 
if (CI && (CI->getParent() == I->getParent())) {
     EVT CmpVT = TLI.getValueType(DL, CmpLHS->getType());
     if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT, CI->getDebugLoc()))
       return false;
   } else {
     unsigned CondReg = getRegForValue(Cond);
     if (CondReg == 0)
       return false;
     bool CondIsKill = hasTrivialKill(Cond);
+
+    // In case OpReg is a K register, kortest against itself.
+    if (MRI.getRegClass(CondReg) == &X86::VK1RegClass)
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+              TII.get(X86::KORTESTWrr))
+          .addReg(CondReg, getKillRegState(CondIsKill))
+          .addReg(CondReg);
+    else
       BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(X86::TEST8ri))
-      .addReg(CondReg, getKillRegState(CondIsKill)).addImm(1);
+          .addReg(CondReg, getKillRegState(CondIsKill))
+          .addImm(1);
   }
 
   const Value *LHS = I->getOperand(1);
   const Value *RHS = I->getOperand(2);
 
   unsigned LHSReg = getRegForValue(LHS);
   bool LHSIsKill = hasTrivialKill(LHS);
 

llvm/trunk/test/CodeGen/X86/fast-isel-select-cmov.ll

-; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10                  | FileCheck %s
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10                  | FileCheck %s --check-prefix=CHECK --check-prefix=NOAVX512
+; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=x86_64-apple-darwin10 -mattr=+avx512f  | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512
 
 ; Test conditional move for the supported types (i16, i32, and i32) and
 ; conditon input (argument or cmp). Currently i8 is not supported.
 
 define zeroext i16 @select_cmov_i16(i1 zeroext %cond, i16 zeroext %a, i16 zeroext %b) {
-; CHECK-LABEL: select_cmov_i16
-; CHECK:       testb   $1, %dil
-; CHECK-NEXT:  cmovew  %dx, %si
-; CHECK-NEXT:  movzwl  %si, %eax
+; NOAVX512-LABEL: select_cmov_i16:
+; NOAVX512:       ## BB#0:
+; NOAVX512-NEXT:    testb $1, %dil
+; NOAVX512-NEXT:    cmovew %dx, %si
+; NOAVX512-NEXT:    movzwl %si, %eax
+; NOAVX512-NEXT:    retq
+;
+; AVX512-LABEL: select_cmov_i16:
+; AVX512:       ## BB#0:
+; AVX512-NEXT:    kmovw %edi, %k0
+; AVX512-NEXT:    kortestw %k0, %k0
+; AVX512-NEXT:    cmovew %dx, %si
+; AVX512-NEXT:    movzwl %si, %eax
+; AVX512-NEXT:    retq
   %1 = select i1 %cond, i16 %a, i16 %b
   ret i16 %1
 }
 
 define zeroext i16 @select_cmp_cmov_i16(i16 zeroext %a, i16 zeroext %b) {
-; CHECK-LABEL: select_cmp_cmov_i16
-; CHECK:       cmpw    %si, %di
-; CHECK-NEXT:  cmovbw  %di, %si
-; CHECK-NEXT:  movzwl  %si, %eax
+; CHECK-LABEL: select_cmp_cmov_i16:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    cmpw %si, %di
+; CHECK-NEXT:    cmovbw %di, %si
+; CHECK-NEXT:    movzwl %si, %eax
+; CHECK-NEXT:    retq
   %1 = icmp ult i16 %a, %b
   %2 = select i1 %1, i16 %a, i16 %b
   ret i16 %2
 }
 
 define i32 @select_cmov_i32(i1 zeroext %cond, i32 %a, i32 %b) {
-; CHECK-LABEL: select_cmov_i32
-; CHECK:       testb   $1, %dil
-; CHECK-NEXT:  cmovel  %edx, %esi
-; CHECK-NEXT:  movl    %esi, %eax
+; NOAVX512-LABEL: select_cmov_i32:
+; NOAVX512:       ## BB#0:
+; NOAVX512-NEXT:    testb $1, %dil
+; NOAVX512-NEXT:    cmovel %edx, %esi
+; NOAVX512-NEXT:    movl %esi, %eax
+; NOAVX512-NEXT:    retq
+;
+; AVX512-LABEL: select_cmov_i32:
+; AVX512:       ## BB#0:
+; AVX512-NEXT:    kmovw %edi, %k0
+; AVX512-NEXT:    kortestw %k0, %k0
+; AVX512-NEXT:    cmovel %edx, %esi
+; AVX512-NEXT:    movl %esi, %eax
+; AVX512-NEXT:    retq
   %1 = select i1 %cond, i32 %a, i32 %b
   ret i32 %1
 }
 
 define i32 @select_cmp_cmov_i32(i32 %a, i32 %b) {
-; CHECK-LABEL: select_cmp_cmov_i32
-; CHECK:       cmpl    %esi, %edi
-; CHECK-NEXT:  cmovbl  %edi, %esi
-; CHECK-NEXT:  movl    %esi, %eax
+; CHECK-LABEL: select_cmp_cmov_i32:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    cmpl %esi, %edi
+; CHECK-NEXT:    cmovbl %edi, %esi
+; CHECK-NEXT:    movl %esi, %eax
+; CHECK-NEXT:    retq
   %1 = icmp ult i32 %a, %b
   %2 = select i1 %1, i32 %a, i32 %b
   ret i32 %2
 }
 
 define i64 @select_cmov_i64(i1 zeroext %cond, i64 %a, i64 %b) {
-; CHECK-LABEL: select_cmov_i64
-; CHECK:       testb   $1, %dil
-; CHECK-NEXT:  cmoveq  %rdx, %rsi
-; CHECK-NEXT:  movq    %rsi, %rax
+; NOAVX512-LABEL: select_cmov_i64:
+; NOAVX512:       ## BB#0:
+; NOAVX512-NEXT:    testb $1, %dil
+; NOAVX512-NEXT:    cmoveq %rdx, %rsi
+; NOAVX512-NEXT:    movq %rsi, %rax
+; NOAVX512-NEXT:    retq
+;
+; AVX512-LABEL: select_cmov_i64:
+; AVX512:       ## BB#0:
+; AVX512-NEXT:    kmovw %edi, %k0
+; AVX512-NEXT:    kortestw %k0, %k0
+; AVX512-NEXT:    cmoveq %rdx, %rsi
+; AVX512-NEXT:    movq %rsi, %rax
+; AVX512-NEXT:    retq
   %1 = select i1 %cond, i64 %a, i64 %b
   ret i64 %1
 }
 
 define i64 @select_cmp_cmov_i64(i64 %a, i64 %b) {
-; CHECK-LABEL: select_cmp_cmov_i64
-; CHECK:       cmpq    %rsi, %rdi
-; CHECK-NEXT:  cmovbq  %rdi, %rsi
-; CHECK-NEXT:  movq    %rsi, %rax
+; CHECK-LABEL: select_cmp_cmov_i64:
+; CHECK:       ## BB#0:
+; CHECK-NEXT:    cmpq %rsi, %rdi
+; CHECK-NEXT:    cmovbq %rdi, %rsi
+; CHECK-NEXT:    movq %rsi, %rax
+; CHECK-NEXT:    retq
   %1 = icmp ult i64 %a, %b
   %2 = select i1 %1, i64 %a, i64 %b
   ret i64 %2
 }