[PATCH] D124743: [DAGCombine] Add node in the worklist in topological order in CombineTo

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added inline comments.


================
Comment at: llvm/test/CodeGen/X86/movmsk-cmp.ll:4564
+; AVX1OR2-NEXT:    testb $1, %cl
+; AVX1OR2-NEXT:    cmovel %edx, %eax
 ; AVX1OR2-NEXT:    retq
----------------
RKSimon wrote:
> deadalnix wrote:
> > I investigated this regression.
> > 
> > before this patch, the DAG after type legalization and optimization looks like:
> > ```SelectionDAG has 25 nodes:
> >   t0: ch = EntryToken
> >             t62: i8 = xor t52, Constant:i8<-1>
> >           t55: i8 = and t62, Constant:i8<1>
> >           t53: i8 = and t52, Constant:i8<2>
> >         t56: i8 = or t55, t53
> >       t57: i8 = setcc t56, Constant:i8<2>, seteq:ch
> >     t40: i32 = select t57, Constant:i32<42>, Constant:i32<99>
> >   t19: ch,glue = CopyToReg t0, Register:i32 $eax, t40
> >         t2: v2f64,ch = CopyFromReg t0, Register:v2f64 %0
> >         t4: v2f64,ch = CopyFromReg t0, Register:v2f64 %1
> >       t41: v2i64 = setcc t2, t4, setogt:ch
> >     t24: i32 = X86ISD::MOVMSK t41
> >   t52: i8 = truncate t24
> >   t20: ch = X86ISD::RET_FLAG t19, TargetConstant:i32<0>, Register:i32 $eax, t19:1
> > ```
> > 
> > After, it looks like:
> > ```
> > SelectionDAG has 20 nodes:
> >   t0: ch = EntryToken
> >       t55: i8 = and t46, Constant:i8<1>
> >         t56: i8 = srl t46, Constant:i8<1>
> >       t21: i32 = select t56, Constant:i32<42>, Constant:i32<99>
> >     t22: i32 = select t55, t21, Constant:i32<99>
> >   t19: ch,glue = CopyToReg t0, Register:i32 $eax, t22
> >         t2: v2f64,ch = CopyFromReg t0, Register:v2f64 %0
> >         t4: v2f64,ch = CopyFromReg t0, Register:v2f64 %1
> >       t40: v2i64 = setcc t2, t4, setogt:ch
> >     t24: i32 = X86ISD::MOVMSK t40
> >   t46: i8 = truncate t24
> >   t20: ch = X86ISD::RET_FLAG t19, TargetConstant:i32<0>, Register:i32 $eax, t19:1
> > ```
> > 
> > Arguably, the DAG after this patch looks simpler/better, so it is hard to argue this does the wrong thing here. However, the rest of the backend doesn't seems to be able to do as good of a job on this DAG. Is there something that can be done here? It seems like there is a legitimate blind spot in the X86 backend rather than a problem with this patch. @RKSimon maybe?
> Adding a select(c2, select(c1, x, y), y) fold is possible.
> 
> Another option is to just alter the test - InstCombine will now optimize this code to a reduction pattern: https://simd.godbolt.org/z/hfv3YW4Y6
> ```
> define i32 @PR39665_c_ray(<2 x double> %x, <2 x double> %y) {
>   %cmp = fcmp ogt <2 x double> %x, %y
>   %shift = shufflevector <2 x i1> %cmp, <2 x i1> poison, <2 x i32> <i32 1, i32 undef>
>   %1 = and <2 x i1> %cmp, %shift
>   %u = extractelement <2 x i1> %1, i64 0
>   %r = select i1 %u, i32 42, i32 99
>   ret i32 %r
> }
> ```
There is already a fold for select(c2, select(c1, x, y), y) , but the X86 backend tell DAGCombine that it prefers the double select. I tried to force the transform, and I got a mixed bag out of it. Sometime it improve things, sometime it makes them worse. Overall, the result was unclear.

I'll put up a patch with the updated IR then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124743