[X86] Avoid using high register trick for test instruction

[deadalnix]

Summary

It seems it's main effect is to create addition copies when values are inr register that do not support this trick, which increase register pressure and makes the code bigger.

The main noteworthy regression I was able to observe was pattern of the type (setcc (trunc (and X, C)), 0) where C is such as it would benefit from the hi register trick. To prevent this, a new pattern is added to materialize such pattern using a 32 bits test. This has the added benefit of working with any constant that is materializable as a 32bits immediate, not just the ones that can leverage the high register trick, as demonstrated by the test case in test-shrink.ll using the constant 2049 .

[craig.topper]

lib/Target/X86/X86ISelDAGToDAG.cpp

3090

I believe this was the only use of the TEST8ri_NOREX instruction. Can you remove it from the td file too?

[niravd]

lib/Target/X86/X86ISelDAGToDAG.cpp

3077

We're now generating a testl but we've passed the logic to reduce to the smaller to testw. Can you fold that logic into this and add a test case (e.g. replicate testOperand32 with optsize set)?

[deadalnix]

Remove TEST8ri_NOREX
Merge codepath for testl and testw

[deadalnix]

test/CodeGen/X86/test-shrink.ll

530

The test case for size optimization was added here in another commit, in case reviewers are wondering.

[niravd]

You're missing the optsize test case but otherwise LGTM.

[deadalnix]

Remove leftover DEBUG

[reames]

Is it worth considering doing this as a late peephole? (i.e. after register allocation if the register happens to be appropriate?)

[craig.topper]

Potentially, but the flag usage is tricky. We can't turn TEST32 into TEST8 if the sign flag is being used. We are guaranteed safe if bit 15 of the immediate is clear. But if bit 15 is set we'd need to check flag users.

[craig.topper]

Reopening since it was reverted. I think this patch experienced some scope creep. As titled it should have only removed the TEST8ri_NOREX creation. Improvements to truncand32 from test-shrink.ll should have been separate based on the title. I don't think there was any need to merge TEST32 and TEST16 handling together especially since TEST8 is still separate. I suspect the bug that caused the revert was created during that merging.

[deadalnix]

Reduce the diff to simply avoid doing the high register trick.
It turns out that there is a bug in the other optimization made in the orginal diff, so it seems like a better idea to split that out in several smaller steps to ensure progress.

[niravd]

LGTM.

lib/Target/X86/X86ISelDAGToDAG.cpp

if ((N0.getOpcode() == ISD::AND ||
         // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
         // one, do not call ReplaceAllUsesWith.
         ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
                     SDValue(NewNode, 0));
         CurDAG->RemoveDeadNode(Node);
         return;
       }
 
-      // For example, "testl %eax, $2048" to "testb %ah, $8".
-      if (isShiftedUInt<8, 8>(Mask) &&
-          (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
-        // Shift the immediate right by 8 bits.
-        SDValue ShiftedImm = CurDAG->getTargetConstant(Mask >> 8, dl, MVT::i8);
-        SDValue Reg = N0.getOperand(0);
-
-        // Extract the h-register.
-        SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_8bit_hi, dl,
-                                                        MVT::i8, Reg);
-
-        // Emit a testb.  The EXTRACT_SUBREG becomes a COPY that can only
-        // target GR8_NOREX registers, so make sure the register class is
-        // forced.
-        SDNode *NewNode = CurDAG->getMachineNode(X86::TEST8ri_NOREX, dl,
-                                                 MVT::i32, Subreg, ShiftedImm);
-        // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
-        // one, do not call ReplaceAllUsesWith.
-        ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
-                    SDValue(NewNode, 0));
-        CurDAG->RemoveDeadNode(Node);
-        return;
-      }
+      // For example, "testq %rax, $268468232" to "testl %eax, $268468232".
+      if (isUInt<32>(Mask) &&
+          (!(Mask & 0x80000000) || hasNoSignedComparisonUses(Node))) {
+        MVT VT = MVT::i32;
+        int SubRegOp = X86::sub_32bit;
+        unsigned Op = X86::TEST32ri;
 
         // For example, "testl %eax, $32776" to "testw %ax, $32776".
         // NOTE: We only want to form TESTW instructions if optimizing for
         // min size. Otherwise we only save one byte and possibly get a length
         // changing prefix penalty in the decoders.
-      if (OptForMinSize && isUInt<16>(Mask) && N0.getValueType() != MVT::i16 &&
+        if (OptForMinSize && isUInt<16>(Mask) &&
             (!(Mask & 0x8000) || hasNoSignedComparisonUses(Node))) {
-        SDValue Imm = CurDAG->getTargetConstant(Mask, dl, MVT::i16);
-        SDValue Reg = N0.getOperand(0);
-
-        // Extract the 16-bit subregister.
-        SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_16bit, dl,
-                                                        MVT::i16, Reg);
-
-        // Emit a testw.
-        SDNode *NewNode = CurDAG->getMachineNode(X86::TEST16ri, dl, MVT::i32,
-                                                 Subreg, Imm);
-        // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
-        // one, do not call ReplaceAllUsesWith.
-        ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
-                    SDValue(NewNode, 0));
-        CurDAG->RemoveDeadNode(Node);
-        return;
+          VT = MVT::i16;
+          SubRegOp = X86::sub_16bit;
+          Op = X86::TEST16ri;
         }
 
-      // For example, "testq %rax, $268468232" to "testl %eax, $268468232".
-      if (isUInt<32>(Mask) && N0.getValueType() == MVT::i64 &&
-          (!(Mask & 0x80000000) || hasNoSignedComparisonUses(Node))) {
-        SDValue Imm = CurDAG->getTargetConstant(Mask, dl, MVT::i32);
+        SDValue Imm = CurDAG->getTargetConstant(Mask, dl, VT);
         SDValue Reg = N0.getOperand(0);
 
-        // Extract the 32-bit subregister.
-        SDValue Subreg = CurDAG->getTargetExtractSubreg(X86::sub_32bit, dl,
-                                                        MVT::i32, Reg);
+        // Extract the subregister if necessary.
+        if (N0.getValueType() != VT)
+          Reg = CurDAG->getTargetExtractSubreg(SubRegOp, dl, VT, Reg);
 
-        // Emit a testl.
-        SDNode *NewNode = CurDAG->getMachineNode(X86::TEST32ri, dl, MVT::i32,
-                                                 Subreg, Imm);
+        // Emit a testl or testw.
+        SDNode *NewNode = CurDAG->getMachineNode(Op, dl, MVT::i32, Reg, Imm);
         // Replace SUB|CMP with TEST, since SUB has two outputs while TEST has
         // one, do not call ReplaceAllUsesWith.
         ReplaceUses(SDValue(Node, (Opcode == X86ISD::SUB ? 1 : 0)),
                     SDValue(NewNode, 0));
         CurDAG->RemoveDeadNode(Node);
         return;
       }
     }

lib/Target/X86/X86InstrArithmetic.td

let Defs = [EFLAGS] in {
     let Predicates = [In64BitMode] in
     def TEST64ri32 : BinOpRI_F<0xF6, "test", Xi64, X86testpat, MRM0r>;
 
     def TEST8mi    : BinOpMI_F<0xF6, "test", Xi8 , X86testpat, MRM0m>;
     def TEST16mi   : BinOpMI_F<0xF6, "test", Xi16, X86testpat, MRM0m>;
     def TEST32mi   : BinOpMI_F<0xF6, "test", Xi32, X86testpat, MRM0m>;
     let Predicates = [In64BitMode] in
     def TEST64mi32 : BinOpMI_F<0xF6, "test", Xi64, X86testpat, MRM0m>;
-
-    // When testing the result of EXTRACT_SUBREG sub_8bit_hi, make sure the
-    // register class is constrained to GR8_NOREX. This pseudo is explicitly
-    // marked side-effect free, since it doesn't have an isel pattern like
-    // other test instructions.
-    let isPseudo = 1, hasSideEffects = 0 in
-    def TEST8ri_NOREX : I<0, Pseudo, (outs), (ins GR8_NOREX:$src, i8imm:$mask),
-                          "", [], IIC_BIN_NONMEM>, Sched<[WriteALU]>;
   } // Defs = [EFLAGS]
 
   def TEST8i8    : BinOpAI_F<0xA8, "test", Xi8 , AL,
                              "{$src, %al|al, $src}">;
   def TEST16i16  : BinOpAI_F<0xA8, "test", Xi16, AX,
                              "{$src, %ax|ax, $src}">;
   def TEST32i32  : BinOpAI_F<0xA8, "test", Xi32, EAX,
                              "{$src, %eax|eax, $src}">;

lib/Target/X86/X86InstrInfo.cpp

case X86::VMOVUPSZ128mr_NOVLX:
     return expandNOVLXStore(MIB, &getRegisterInfo(), get(X86::VMOVUPSmr),
                             get(X86::VEXTRACTF32x4Zmr), X86::sub_xmm);
   case X86::VMOVAPSZ256mr_NOVLX:
     return expandNOVLXStore(MIB, &getRegisterInfo(), get(X86::VMOVAPSYmr),
                             get(X86::VEXTRACTF64x4Zmr), X86::sub_ymm);
   case X86::VMOVUPSZ256mr_NOVLX:
     return expandNOVLXStore(MIB, &getRegisterInfo(), get(X86::VMOVUPSYmr),
                             get(X86::VEXTRACTF64x4Zmr), X86::sub_ymm);
-  case X86::TEST8ri_NOREX:
-    MI.setDesc(get(X86::TEST8ri));
-    return true;
   case X86::MOV32ri64:
     MI.setDesc(get(X86::MOV32ri));
     return true;
 
   // KNL does not recognize dependency-breaking idioms for mask registers,
   // so kxnor %k1, %k1, %k2 has a RAW dependence on %k1.
   // Using %k0 as the undef input register is a performance heuristic based
   // on the assumption that %k0 is used less frequently than the other mask

lib/Target/X86/X86MacroFusion.cpp

static bool shouldScheduleAdjacent(const TargetInstrInfo &TII,
   case X86::TEST32ri:
   case X86::TEST32i32:
   case X86::TEST64i32:
   case X86::TEST64ri32:
   case X86::TEST8mr:
   case X86::TEST16mr:
   case X86::TEST32mr:
   case X86::TEST64mr:
-  case X86::TEST8ri_NOREX:
   case X86::AND16i16:
   case X86::AND16ri:
   case X86::AND16ri8:
   case X86::AND16rm:
   case X86::AND16rr:
   case X86::AND32i32:
   case X86::AND32ri:
   case X86::AND32ri8:

test/CodeGen/X86/test-shrink.ll

yes:
   ret void
 no:
   ret void
 }
 
 define void @truncand32(i16 inreg %x) nounwind {
 ; CHECK-LINUX64-LABEL: truncand32:
 ; CHECK-LINUX64:       # %bb.0:
-; CHECK-LINUX64-NEXT:    andl $2049, %edi # imm = 0x801
-; CHECK-LINUX64-NEXT:    testw %di, %di
+; CHECK-LINUX64-NEXT:    testl $2049, %edi # imm = 0x801
 ; CHECK-LINUX64-NEXT:    je .LBB11_1
 ; CHECK-LINUX64-NEXT:  # %bb.2: # %no
 ; CHECK-LINUX64-NEXT:    retq
 ; CHECK-LINUX64-NEXT:  .LBB11_1: # %yes
 ; CHECK-LINUX64-NEXT:    pushq %rax
 ; CHECK-LINUX64-NEXT:    callq bar
 ; CHECK-LINUX64-NEXT:    popq %rax
 ; CHECK-LINUX64-NEXT:    retq
 ;
 ; CHECK-WIN32-64-LABEL: truncand32:
 ; CHECK-WIN32-64:       # %bb.0:
 ; CHECK-WIN32-64-NEXT:    subq $40, %rsp
-; CHECK-WIN32-64-NEXT:    andl $2049, %ecx # imm = 0x801
-; CHECK-WIN32-64-NEXT:    testw %cx, %cx
+; CHECK-WIN32-64-NEXT:    testl $2049, %ecx # imm = 0x801
 ; CHECK-WIN32-64-NEXT:    je .LBB11_1
 ; CHECK-WIN32-64-NEXT:  # %bb.2: # %no
 ; CHECK-WIN32-64-NEXT:    addq $40, %rsp
 ; CHECK-WIN32-64-NEXT:    retq
 ; CHECK-WIN32-64-NEXT:  .LBB11_1: # %yes
 ; CHECK-WIN32-64-NEXT:    callq bar
 ; CHECK-WIN32-64-NEXT:    addq $40, %rsp
 ; CHECK-WIN32-64-NEXT:    retq
 ;
 ; CHECK-X86-LABEL: truncand32:
 ; CHECK-X86:       # %bb.0:
-; CHECK-X86-NEXT:    andl $2049, %eax # imm = 0x801
-; CHECK-X86-NEXT:    testw %ax, %ax
+; CHECK-X86-NEXT:    testl $2049, %eax # imm = 0x801
 ; CHECK-X86-NEXT:    je .LBB11_1
 ; CHECK-X86-NEXT:  # %bb.2: # %no
 ; CHECK-X86-NEXT:    retl
 ; CHECK-X86-NEXT:  .LBB11_1: # %yes
 ; CHECK-X86-NEXT:    calll bar
 ; CHECK-X86-NEXT:    retl
   %t = and i16 %x, 2049
   %s = icmp eq i16 %t, 0
   br i1 %s, label %yes, label %no
 
 yes:
   call void @bar()
   ret void
 no:
   ret void
 }
 
 define void @testw(i16 inreg %x) nounwind minsize {
 ; CHECK-LINUX64-LABEL: testw:
 ; CHECK-LINUX64:       # %bb.0:
 ; CHECK-LINUX64-NEXT:    testw $2049, %di # imm = 0x801
 ; CHECK-LINUX64-NEXT:    je .LBB12_1
 ; CHECK-LINUX64-NEXT:  # %bb.2: # %no
 ; CHECK-LINUX64-NEXT:    retq
 ; CHECK-LINUX64-NEXT:  .LBB12_1: # %yes
 ; CHECK-LINUX64-NEXT:    pushq %rax

test/CodeGen/X86/testb-je-fusion.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-- -mcpu=corei7-avx | FileCheck %s
 
 ; testb should be scheduled right before je to enable macro-fusion.
 
 define i32 @check_flag(i32 %flags, ...) nounwind {
 ; CHECK-LABEL: check_flag:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    movl %edi, %ecx
 ; CHECK-NEXT:    xorl %eax, %eax
-; CHECK-NEXT:    testb $2, %ch
+; CHECK-NEXT:    testl $512, %edi # imm = 0x200
 ; CHECK-NEXT:    je .LBB0_2
 ; CHECK-NEXT:  # %bb.1: # %if.then
 ; CHECK-NEXT:    movl $1, %eax
 ; CHECK-NEXT:  .LBB0_2: # %if.end
 ; CHECK-NEXT:    retq
 entry:
   %and = and i32 %flags, 512
   %tobool = icmp eq i32 %and, 0

test/CodeGen/X86/vastart-defs-eflags.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc %s -o - | FileCheck %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.10.0"
 
 ; Check that vastart handling doesn't get between testb and je for the branch.
 define i32 @check_flag(i32 %flags, ...) nounwind {
 ; CHECK-LABEL: check_flag:
 ; CHECK:       ## %bb.0: ## %entry
-; CHECK-NEXT:    pushq %rbx
-; CHECK-NEXT:    subq $48, %rsp
-; CHECK-NEXT:    movl %edi, %ebx
+; CHECK-NEXT:    subq $56, %rsp
 ; CHECK-NEXT:    testb %al, %al
 ; CHECK-NEXT:    je LBB0_2
 ; CHECK-NEXT:  ## %bb.1: ## %entry
 ; CHECK-NEXT:    movaps %xmm0, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movaps %xmm1, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movaps %xmm2, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movaps %xmm3, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movaps %xmm4, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movaps %xmm5, (%rsp)
 ; CHECK-NEXT:    movaps %xmm6, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movaps %xmm7, {{[0-9]+}}(%rsp)
 ; CHECK-NEXT:  LBB0_2: ## %entry
 ; CHECK-NEXT:    movq %r9, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movq %r8, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movq %rcx, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movq %rdx, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    movq %rsi, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    xorl %eax, %eax
-; CHECK-NEXT:    testb $2, %bh
+; CHECK-NEXT:    testl $512, %edi ## imm = 0x200
 ; CHECK-NEXT:    je LBB0_4
 ; CHECK-NEXT:  ## %bb.3: ## %if.then
 ; CHECK-NEXT:    leaq -{{[0-9]+}}(%rsp), %rax
 ; CHECK-NEXT:    movq %rax, 16
 ; CHECK-NEXT:    leaq {{[0-9]+}}(%rsp), %rax
 ; CHECK-NEXT:    movq %rax, 8
 ; CHECK-NEXT:    movl $48, 4
 ; CHECK-NEXT:    movl $8, 0
 ; CHECK-NEXT:    movl $1, %eax
 ; CHECK-NEXT:  LBB0_4: ## %if.end
-; CHECK-NEXT:    addq $48, %rsp
-; CHECK-NEXT:    popq %rbx
+; CHECK-NEXT:    addq $56, %rsp
 ; CHECK-NEXT:    retq
 entry:
   %and = and i32 %flags, 512
   %tobool = icmp eq i32 %and, 0
   br i1 %tobool, label %if.end, label %if.then
 
 if.then:                                          ; preds = %entry
   call void @llvm.va_start(i8* null)
   br label %if.end
 
 if.end:                                           ; preds = %entry, %if.then
   %hasflag = phi i32 [ 1, %if.then ], [ 0, %entry ]
   ret i32 %hasflag
 }
 
 declare void @llvm.va_start(i8*) nounwind