[PATCH] D106684: [x86] improve CMOV codegen by pushing add into operands, part 2

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/test/CodeGen/X86/add-cmov.ll:158-159
 ; CHECK-NEXT:    retq
   %s = select i1 %b, i64 42, i64 2147483648
   %r = add i64 %offset, %s
   ret i64 %r
----------------
lebedev.ri wrote:
> Would transforming this into
> ```
>   %tval = add i64 %offset, 42
>   %fval = add i64 %tval, 2147483606
>   %r = select i1 %b, i64 %tval, i64 %fval
> ```
> be a win? https://godbolt.org/z/rYjzePnr4
Yes, it seems likely to be a win if we can combine the adds sequentially like that. We should check how often we see large constants like this before making the transform more complicated though. I don't have stats on it, but seems unlikely?
 
This is the example that I was referring to when I updated the patch - we change the add to sub somewhere in X86SelDAGToDAG causing a seemingly extra reg copy later on:
	
```
movq	%rdi, %rax
movq	%rdi, %rcx
subq	$-2147483648, %rcx              ## imm = 0x80000000
addq	$42, %rax
testb	$1, %sil
cmoveq	%rcx, %rax
```


================
Comment at: llvm/test/CodeGen/X86/add-cmov.ll:251
+; CHECK-NEXT:    cmovneq %rcx, %rax
 ; CHECK-NEXT:    retq
   %gep2 = getelementptr inbounds %class.btAxis, %class.btAxis* %ptr, i64 %idx, i32 2, i64 0
----------------
RKSimon wrote:
> I think this could be: https://llvm.godbolt.org/z/WMaPvfKKh
> ```
>   leaq (%rdx,%rdx,4), %rax
>   shlq $4, %rax
>   leaq 6(%rax), %rcx
>   testb $1, %dil
>   cmovneq %rax, %rcx
>   leaq 60(%rsi,%rcx), %rax
> ```
We would need to factor out the common ops in the expressions and move it after the select...not sure what layer to try that yet. I think that's the same as what you showed in https://llvm.org/PR51069, I'll put this into the bug report, so we have another example to look at.


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

https://reviews.llvm.org/D106684