[X86] Emit testl instead of testb for select condition where possible

[aivchenk]

Summary

For simple cases like:

define i32 @select_0_or_neg1_zeroext(i1 zeroext %cond) {

%sel = select i1 %cond, i32 0, i32 -1
ret i32 %sel

}

..we stopped generating "testl %edi,%edi" and started generating
"testb %dil,%dil". testl encoding is 1-byte smaller than testb,
so it is preferable.
This happens since https://reviews.llvm.org/D32273 where condition
for example's select started to look like:
t7: i8 = AssertZext t5, ValueType:ch:i1

t5: i8 = truncate t4
  t4: i32 = AssertZext t2, ValueType:ch:i8
    t2: i32,ch = CopyFromReg t0, Register:i32 %vreg0

The patch fixes that by directly checking that pattern in
X86TargetLowering::LowerSELECT. The example is taken directly
from test/CodeGen/X86/select_const.ll, so the tests are already
in place.

[spatel]

I haven't looked at AssertZext before, so I'm going to learn something today. :)
I'm curious about this sequence:

      t4: i32 = AssertZext t2, ValueType:ch:i8
    t5: i8 = truncate t4
  t7: i8 = AssertZext t5, ValueType:ch:i1
t8: i1 = truncate t7

1. If an AssertZext (t7) has a truncate (t5) source operand with no other uses, can we reverse those ops (effectively, we make the assert stronger)?
2. This leads to another question: if an AssertZext is preceded by a weaker AssertZext, can we eliminate the weaker AssertZext?

FWIW, I drafted #1, and it solves this bug (we get the 32-bit ops you're getting here). It also creates several other diffs. At first glance, they look like x86 improvements to me, but I could use a 2nd opinion on that.
If that's a reasonable generic DAG transform, I'd prefer to do that rather than complicating the x86-specific code even more.

[aivchenk]

yeah, now I also think that I covered only a narrow case for the issue: we need more general approach.

My 2 cents on your points:

1. If the result of AssertZext is truncated with no other uses, can we just delete that AssertZext? What value does it bring if all bits that are supposed to be zeroed just being thrown away? Or the case here would be that zero extend was e.g. to 32 bits but we truncate e.g. to 16? Could you please share the x86 test changes after your patch?
2. I think this is the right thing to do :)

[spatel]

In D36890#848823, @aivchenk wrote:

yeah, now I also think that I covered only a narrow case for the issue: we need more general approach.

My 2 cents on your points:

1. If the result of AssertZext is truncated with no other uses, can we just delete that AssertZext?

I don't think we want to do that. IIUC, the point of the AssertZext is to preserve 'zeroext' info on values that would otherwise be lost during legalization/lowering. If we delete the AssertZext, we'll probably produce worse code?

Could you please share the x86 test changes after your patch?

Sure - let me clean up the draft patch and post it for review.

[aivchenk]

D37017 is a better way of fixing the issue. Hence, closing

lib/Target/X86/X86ISelLowering.cpp

if (CondOpcode == X86ISD::SETCC ||
     else
       Cond = X86Op.getValue(1);
 
     CC = DAG.getConstant(X86Cond, DL, MVT::i8);
     AddTest = false;
   }
 
   if (AddTest) {
+    // Look past the AssertZext to see whether it is a truncate operation.
+    if (Cond.getOpcode() == ISD::AssertZext &&
+        Cond.getOperand(0).getOpcode() == ISD::TRUNCATE)
+      Cond = Cond.getOperand(0);
+
     // Look past the truncate if the high bits are known zero.
     if (isTruncWithZeroHighBitsInput(Cond, DAG))
       Cond = Cond.getOperand(0);
 
     // We know the result of AND is compared against zero. Try to match
     // it to BT.
     if (Cond.getOpcode() == ISD::AND && Cond.hasOneUse()) {
       if (SDValue NewSetCC = LowerToBT(Cond, ISD::SETNE, DL, DAG)) {

test/CodeGen/X86/fp128-select.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+mmx \
 ; RUN:     -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
 ; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+mmx \
 ; RUN:     -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
 ; RUN: llc < %s -O2 -mtriple=x86_64-linux-android \
 ; RUN:     -enable-legalize-types-checking | FileCheck %s
 ; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu \
 ; RUN:     -enable-legalize-types-checking | FileCheck %s
 
 define void @test_select(fp128* %p, fp128* %q, i1 zeroext %c) {
 ; MMX-LABEL: test_select:
 ; MMX:       # BB#0:
-; MMX-NEXT:    testb %dl, %dl
+; MMX-NEXT:    testl %edx, %edx
 ; MMX-NEXT:    jne .LBB0_1
 ; MMX-NEXT:  # BB#2:
 ; MMX-NEXT:    movaps {{.*}}(%rip), %xmm0
 ; MMX-NEXT:    movaps %xmm0, (%rsi)
 ; MMX-NEXT:    retq
 ; MMX-NEXT:  .LBB0_1:
 ; MMX-NEXT:    movaps (%rdi), %xmm0
 ; MMX-NEXT:    movaps %xmm0, (%rsi)
 ; MMX-NEXT:    retq
 ;
 ; CHECK-LABEL: test_select:
 ; CHECK:       # BB#0:
 ; CHECK-NEXT:    xorl %eax, %eax
-; CHECK-NEXT:    testb %dl, %dl
+; CHECK-NEXT:    testl %edx, %edx
 ; CHECK-NEXT:    cmovneq (%rdi), %rax
 ; CHECK-NEXT:    movabsq $9223231299366420480, %rcx # imm = 0x7FFF800000000000
 ; CHECK-NEXT:    cmovneq 8(%rdi), %rcx
 ; CHECK-NEXT:    movq %rcx, 8(%rsi)
 ; CHECK-NEXT:    movq %rax, (%rsi)
 ; CHECK-NEXT:    retq
   %a = load fp128, fp128* %p, align 2
   %r = select i1 %c, fp128 %a, fp128 0xL00000000000000007FFF800000000000
   store fp128 %r, fp128* %q
   ret void
 }

test/CodeGen/X86/select_const.ll

 ; CHECK-NEXT:    retq
   %sel = select i1 %cond, i32 421, i32 42
   ret i32 %sel
 }
 
 define i32 @select_C1_C2_zeroext(i1 zeroext %cond) {
 ; CHECK-LABEL: select_C1_C2_zeroext:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    testb %dil, %dil
+; CHECK-NEXT:    testl %edi, %edi
 ; CHECK-NEXT:    movl $421, %ecx # imm = 0x1A5
 ; CHECK-NEXT:    movl $42, %eax
 ; CHECK-NEXT:    cmovnel %ecx, %eax
 ; CHECK-NEXT:    retq
   %sel = select i1 %cond, i32 421, i32 42
   ret i32 %sel
 }