[PATCH] D59035: [X86] Promote i8 CMOV's (PR40965)

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

Looks good to me.



================
Comment at: test/CodeGen/X86/cmov-promotion.ll:155-158
+; CMOV-NEXT:    movl $117, %eax
+; CMOV-NEXT:    movl $237, %ecx
+; CMOV-NEXT:    cmovnel %eax, %ecx
+; CMOV-NEXT:    movsbl %cl, %eax
----------------
I noticed that we could avoid the sign extend if we move -19 instead of 237 to ECX, and we commute the operands of that CMOV (along with the condition: from NE to E).

The following sequence should be equivalent:

```
; CMOV-NEXT:    movl $117, %eax
; CMOV-NEXT:    movl $-19, %ecx
; CMOV-NEXT:    cmovel %ecx, %eax
```

Same for other 'spromotion' tests below.

P.s.: none of these things require changes to your patch. This was just FIY (something that I found interesting while looking at the test changes).


================
Comment at: test/CodeGen/X86/copy-eflags.ll:257-265
+; X64-NEXT:    xorl %ecx, %ecx
+; X64-NEXT:    cmpq %rax, %rsi
+; X64-NEXT:    setl %cl
+; X64-NEXT:    negl %ecx
+; X64-NEXT:    cmpq %rax, %rsi
+; X64-NEXT:    movzbl %al, %edi
+; X64-NEXT:    cmovgel %r11d, %edi
----------------
Nice change.
It is a shame that we have to repeat the same CMPQ because of the NEGL which modifies FLAGS. In theory, we could reorder that sequence and avoid to repeat the same compare.
Again, this may not be that important and it has nothing to do with your patch.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59035