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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 5 inline comments as done.
spatel added inline comments.


================
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.
----------------
lebedev.ri wrote:
> It's not really hoisting, `pushAddThroughCmovOfConsts`/`pushAddIntoCmovOfConsts` maybe or something?
Sure - will update.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49872
+  SDLoc DL(N);
+  SDValue FalseOp = Cmov.getOperand(0);
+  SDValue TrueOp = Cmov.getOperand(1);
----------------
lebedev.ri wrote:
> 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.
I copied that code from combineCMov(), so we should be consistent in this file at least.


================
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
----------------
lebedev.ri wrote:
> I guess you need to look past a truncation when looking for the cmov,
> and then widen everything to the cmov type.
Yes - or we try to carve out another case where we prefer i32 instead of trying to narrow the add. Changing the preferred type would probably be the better choice, but there will likely be a lot of test diffs...


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

https://reviews.llvm.org/D106607