[PATCH] D120597: [RISCV] With Zbb, fold (sext_inreg (abs X)) -> (max X, (negw X))

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:7541
+      cast<VTSDNode>(N->getOperand(1))->getVT() == MVT::i32 &&
+      DAG.ComputeNumSignBits(Src.getOperand(0)) > 32) {
+    SDLoc DL(N);
----------------
craig.topper wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > spatel wrote:
> > > > spatel wrote:
> > > > > craig.topper wrote:
> > > > > > spatel wrote:
> > > > > > > Can the `ComputeNumSignBits` be an assert rather than part of the predicate?
> > > > > > The input is sign extended if the abs was promoted by type legalization, but I think it is possible to write (i64 (sext (i32 (trunc (i64 abs X)))) in the original IR and the input would not be sign extended.
> > > > > Maybe I'm not understanding the pattern - is it possible to write a negative test?
> > > > > If we sext_inreg from i32, does this model the transform:
> > > > > https://alive2.llvm.org/ce/z/j4RdVa ?
> > > > I'm still not seeing it after reading the comment/example:
> > > > ashr X, 32   -> adds 32 signbits to at least 1 existing signbit
> > > > How can this be under 33?
> > > > 
> > > > https://alive2.llvm.org/ce/z/Rvk__m
> > > > 
> > > Your shift result isn't being used your src function returned %abs not %ashr  https://alive2.llvm.org/ce/z/eRHZww
> > > 
> > > This is the transform I'm trying to do here
> > > 
> > > ```
> > > define i64 @src(i64 %x) {
> > >   %abs = call i64 @llvm.abs.i64(i64 %x, i1 0)
> > >   %shl = shl i64 %abs, 32
> > >   %ashr = ashr i64 %shl, 32
> > >   ret i64 %ashr
> > > }
> > > 
> > > define i64 @tgt(i64 %x) {
> > >   %f = freeze i64 %x
> > >   %negx = sub i64 0, %f
> > >   %shl = shl i64 %negx, 32
> > >   %ashr = ashr i64 %shl, 32
> > >   %max = call i64 @llvm.smax.i64(i64 %ashr, i64 %negx)
> > >   ret i64 %max
> > > }
> > > ```
> > > 
> > > It's only valid if %x has 33 sign bits.
> > > 
> > > 
> > Oops that's not right. Give me a few minutes
> This is the transform I'm doing here
> 
> ```
> define i64 @src(i64 %x) {
>   %abs = call i64 @llvm.abs.i64(i64 %x, i1 0)
>   %shl = shl i64 %abs, 32
>   %ashr = ashr i64 %shl, 32
>   ret i64 %ashr
> }
> 
> define i64 @tgt(i64 %x) {
>   %f = freeze i64 %x
>   %negx = sub i64 0, %f
>   %shl = shl i64 %negx, 32
>   %ashr = ashr i64 %shl, 32
>   %max = call i64 @llvm.smax.i64(i64 %f, i64 %ashr)
>   ret i64 %max
> }
> ```
> 
> But it doesn't work if %x doesn't have 33 sign bits.
Thanks - now I see it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120597