[PATCH] D55870: [X86] Don't match TESTrr from (cmp (and X, Y), 0) during isel. Defer to post processing

Wed Dec 19 08:31:42 PST 2018

craig.topper marked 2 inline comments as done.
craig.topper added inline comments.

================
Comment at: test/CodeGen/X86/bmi.ll:624-635
 ; X64-LABEL: blsi64_z2:
 ; X64:       # %bb.0:
 ; X64-NEXT:    movq %rsi, %rax
-; X64-NEXT:    movq %rdi, %rcx
-; X64-NEXT:    negq %rcx
-; X64-NEXT:    testq %rdi, %rcx
+; X64-NEXT:    blsiq %rdi, %rcx
 ; X64-NEXT:    cmovneq %rdx, %rax
 ; X64-NEXT:    retq
   %t0 = sub i64 0, %a
----------------
craig.topper wrote:
> andreadb wrote:
> > Again. Here we may prefer POPCNT to BLSI. It tends to have better latency/throughput overall. I think it is worthy to raise a bug for this.
> > 
> > Speaking about these tests in general:
> > I think that we should make these more robust (maybe in a separate patch). 
> > 
> > We can probably make this test more robust by changing how we check the result. For example, rather than comparing against zero, we can compare against a specific power-of-2. That would force the selection of BLSI, since we would need to know the position of that bit.
> > 
> > We can probably do something similar to improve the other test.
> I thought we just established that BLSI could be replaced with a compare of the input with 0. Why would we replace it with POPCNT?
Doesn't BLSI have better throughput than POPCNT on Intel CPUs? BLSI is on 2 ports. POPCNT is only one port.

================
Comment at: test/CodeGen/X86/bmi.ll:880-882
 ; X64-NEXT:    movl %esi, %eax
-; X64-NEXT:    # kill: def $edi killed $edi def $rdi
-; X64-NEXT:    leal -1(%rdi), %ecx
-; X64-NEXT:    testl %edi, %ecx
+; X64-NEXT:    blsrl %edi, %ecx
 ; X64-NEXT:    cmovnel %edx, %eax
----------------
andreadb wrote:
> Same.
> Could be a simple `test+cmov`. But - again - I take that the purpose of this test is not to check if we are smart enough to fold away that sequence...
How could this be a TEST+CMOV? ZF will be set if the input is zero or has exactly one bit set.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55870/new/

https://reviews.llvm.org/D55870