[PATCH] D123109: [x86] improve select lowering for smin(x, 0) & smax(x, 0)

[Wei Xiao via Phabricator via llvm-commits]

wxiao3 added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:24702
+  // (select (x > 0), x, 0) -> (~(x >> (size_in_bits(x)-1))) & x
+  // (select (x < 0), x, 0) -> ((x >> (size_in_bits(x)-1))) & x
   if (Cond.getOpcode() == X86ISD::SETCC &&
----------------
RKSimon wrote:
> What about the more general cases (select (x > 0), y, 0) & (select (x < 0), y, 0)?
> 
> Although I think that requires a freeze:
> ```
> define i8 @src(i8 %x, i8 %y) {
> %0:
>   %c = icmp sge i8 %x, 0
>   %r = select i1 %c, i8 %y, i8 0
>   ret i8 %r
> }
> =>
> define i8 @tgt(i8 %x, i8 %y) {
> %0:
>   %c = ashr i8 %x, 7
>   %m = xor i8 %c, 255
>   %f = freeze i8 %y
>   %r = and i8 %m, %f
>   ret i8 %r
> }
> Transformation seems to be correct!
> ```
you're right that we can make it more general.
I restrict it on purpose to align with [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L23400 | DAGCombiner ]]

so that for following test:

```
$ cat t1.ll
define i32 @test1_ir(i32 %a, i32 %b) nounwind {
  %tmp1 = icmp sgt i32 %a, %b
  %r = select i1 %tmp1, i32 %a, i32 %b
  ret i32 %r
}

define i32 @test1_intrinsic(i32 %a, i32 %b) nounwind {
  %r = call i32 @llvm.smax.i32(i32 %a, i32 %b)
  ret i32 %r
}

define i32 @test2_ir(i32 %a) nounwind {
  %tmp1 = icmp sgt i32 %a, 0
  %r = select i1 %tmp1, i32 %a, i32 0
  ret i32 %r
}

define i32 @test2_intrinsic(i32 %a) nounwind {
  %r = call i32 @llvm.smax.i32(i32 %a, i32 0)
  ret i32 %r
}

declare i32 @llvm.smax.i32(i32, i32)
```
we can generate the same assembly between ir version and intrinsic version.

I'd like to prepare another patch to make it more general according to your suggestions by relaxing the restriction in both here and [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L23400 | DAGCombiner ]] at the same time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123109