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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.


================
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
----------------
lebedev.ri wrote:
> andreadb wrote:
> > 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).
> Given that the input is
> ```
> define i16 @old(i1 %c) {
>   %t0 = select i1 %c, i8 117, i8 -19
>   %ret = sext i8 %t0 to i16
>   ret i16 %ret
> }
> ```
> why can't we simply widen the hands of select, like
> ```
> define i16 @new(i1 %c) {
>   %ret = select i1 %c, i16 117, i16 -19
>   ret i16 %ret
> }
> ```
> https://rise4fun.com/Alive/cs8
> ?
> 
> So instead of 
> ```
>         testb   $1, %dil
>         movl    $117, %eax
>         movl    $237, %ecx
>         cmovnel %eax, %ecx
>         movsbl  %cl, %eax
> 
> ```
> we get
> ```
>         testb   $1, %dil
>         movl    $117, %ecx
>         movl    $65517, %eax            # imm = 0xFFED
>         cmovnel %ecx, %eax
> ```
> 
> I.e. the 'obvious' fix here is that if we are widening result of CMOV,
> and both of the possibilities are constants, then just widen those constants and CMOV itself.
> 
> Why do you think we'd need to flip the CMOV condition?
D59035


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59035