[PATCH] D59001: X86TargetLowering::LowerSELECT(): don't promote CMOV's if the subtarget does't have them

[Roman Lebedev via Phabricator via llvm-commits]

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


================
Comment at: test/CodeGen/X86/midpoint-int.ll:803
+; X32-NEXT:    movl %edx, %edi
+; X32-NEXT:    ja .LBB11_2
+; X32-NEXT:  # %bb.1:
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > craig.topper wrote:
> > > > Do you understand why we went from one conditional branch to two?
> > > I'm not sure yet, but i just want to point out that in all other cases we already had 2 conditional jumps.
> > > In this case we had one conditional jump and one uncondititonal jump, and that regressed to two conditional jumps.
> > Hmm, interesting.
> > ```# *** IR Dump After Expand ISel Pseudo-instructions ***:
> > # Machine code for function scalar_i16_unsigned_reg_reg: IsSSA, TracksLiveness
> > Frame Objects:
> >   fi#-2: size=2, align=4, fixed, at location [SP+8]
> >   fi#-1: size=2, align=4, fixed, at location [SP+4]
> > 
> > bb.0 (%ir-block.0):
> >   successors: %bb.1(0x40000000), %bb.2(0x40000000); %bb.1(50.00%), %bb.2(50.00%)
> > 
> >   %0:gr32 = MOV32rm %fixed-stack.1, 1, $noreg, 0, $noreg :: (load 2 from %fixed-stack.1, align 4)
> >   %1:gr16 = COPY %0.sub_16bit:gr32
> >   %2:gr16 = MOV16rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 2 from %fixed-stack.0, align 4)
> >   %3:gr16 = SUB16rr %1:gr16(tied-def 0), %2:gr16, implicit-def $eflags
> >   %4:gr8 = SETBEr implicit $eflags
> >   %5:gr32_nosp = MOVZX32rr8 killed %4:gr8
> >   %6:gr32 = LEA32r %5:gr32_nosp, 1, %5:gr32_nosp, -1, $noreg
> >   JA_1 %bb.2, implicit $eflags
> > 
> > bb.1 (%ir-block.0):
> > ; predecessors: %bb.0
> >   successors: %bb.2(0x80000000); %bb.2(100.00%)
> >   liveins: $eflags
> > 
> > bb.2 (%ir-block.0):
> > ; predecessors: %bb.0, %bb.1
> >   successors: %bb.3(0x40000000), %bb.4(0x40000000); %bb.3(50.00%), %bb.4(50.00%)
> >   liveins: $eflags
> >   %7:gr16 = PHI %1:gr16, %bb.1, %2:gr16, %bb.0
> >   %9:gr32 = IMPLICIT_DEF
> >   %8:gr32 = INSERT_SUBREG %9:gr32(tied-def 0), killed %7:gr16, %subreg.sub_16bit
> >   JA_1 %bb.4, implicit $eflags
> > 
> > bb.3 (%ir-block.0):
> > ; predecessors: %bb.2
> >   successors: %bb.4(0x80000000); %bb.4(100.00%)
> > 
> > 
> > bb.4 (%ir-block.0):
> > ; predecessors: %bb.2, %bb.3
> > 
> >   %10:gr16 = PHI %2:gr16, %bb.3, %1:gr16, %bb.2
> >   %12:gr32 = IMPLICIT_DEF
> >   %11:gr32 = INSERT_SUBREG %12:gr32(tied-def 0), killed %10:gr16, %subreg.sub_16bit
> >   %13:gr32 = SUB32rr %11:gr32(tied-def 0), killed %8:gr32, implicit-def dead $eflags
> >   %14:gr16 = COPY %13.sub_16bit:gr32
> >   %15:gr32 = MOVZX32rr16 killed %14:gr16
> >   %16:gr32 = SHR32r1 %15:gr32(tied-def 0), implicit-def dead $eflags
> >   %17:gr32 = IMUL32rr %16:gr32(tied-def 0), killed %6:gr32, implicit-def dead $eflags
> >   %18:gr32 = ADD32rr %17:gr32(tied-def 0), %0:gr32, implicit-def dead $eflags
> >   %19:gr16 = COPY %18.sub_16bit:gr32
> >   $ax = COPY %19:gr16
> >   RET 0, $ax
> > 
> > # End machine code for function scalar_i16_unsigned_reg_reg.
> > ```
> > So we indeed have two conditional jumps based on the same condition.
> > But i'm not sure i understand.
> >  * `bb.4` is reachable from conditional jump from `bb.2` OR via a fallthrough from `bb.3`.
> >  * `bb.3` is only reachable via fallthrough from `bb.2`.
> >  * `bb.3` is empty
> >  * Thus, `bb.4` will always be visited if we visited `bb.2`.
> > 
> > This sounds wrong to me (i'm likely not getting something),
> > but why can't we just fold `bb.3` and `bb.4` into `bb.2`?
> Though that of course tries to answer the question of why we have two conditional jumps,
> not why we regress from an unconditional jump to a conditional jump.
I have digged a bit, and while i'm unable to answer *why* this *regresses*,
i do believe this ends up simply exposing an existing missing optimization,
which i have filed as https://bugs.llvm.org/show_bug.cgi?id=40974 with all the data i have.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59001