[PATCH] D106607: [x86] improve CMOV codegen by hoisting add

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Aha, yes, this late this is fine.
Looks good to me.
I guess you want to deal with i8 as a follow-up step?



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49850
+/// earlier folds that may be used to turn select-of-constants into logic hacks.
+static SDValue hoistAddAboveCmovOfConsts(SDNode *N, SelectionDAG &DAG) {
+  // This checks for a zero operand because add-of-0 gets simplified away.
----------------
It's not really hoisting, `pushAddThroughCmovOfConsts`/`pushAddIntoCmovOfConsts` maybe or something?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49872
+  SDLoc DL(N);
+  SDValue FalseOp = Cmov.getOperand(0);
+  SDValue TrueOp = Cmov.getOperand(1);
----------------
craig.topper wrote:
> pengfei wrote:
> > Surprising to see the first operand is false operand. I saw we are taking it as true in same cases
> > ```
> > def : Pat<(f128 (X86cmov VR128:$t, VR128:$f, timm:$cond, EFLAGS)),
> >           (CMOV_VR128 VR128:$t, VR128:$f, timm:$cond)>;
> > ```
> > Which one is right?
> It should be false first. For non-pseudos false should be same as the dest operand making it a tied register which should be the first source operand. The comments in emitLoweredSelect which handle the pseudo also have false first.
Unless i'm really misreading it, i believe the assembly is correct, so false first.


================
Comment at: llvm/test/CodeGen/X86/add-cmov.ll:51-52
 ; CHECK-NEXT:    movl $45, %eax
 ; CHECK-NEXT:    cmovnel %ecx, %eax
 ; CHECK-NEXT:    addb %dil, %al
 ; CHECK-NEXT:    # kill: def $al killed $al killed $eax
----------------
I guess you need to look past a truncation when looking for the cmov,
and then widen everything to the cmov type.


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

https://reviews.llvm.org/D106607