[PATCH] D152911: [PowerPC] Remove extend between shift and and

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp accepted this revision as: stefanp.
stefanp added a comment.

I think this patch looks good.

I'm going to approve it and the nit that I had:

  unsigned NarrowOpcode = NarrowOp.getOpcode();
  if (NarrowOpcode != ISD::SHL && ...

can be addressed on commit.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15475
+    if ((Op1.getOpcode() != ISD::ZERO_EXTEND &&
+         Op1.getOpcode() != ISD::ANY_EXTEND) ||
+        !isa<ConstantSDNode>(Op2))
----------------
nemanjai wrote:
> stefanp wrote:
> > It may be possible to also make this safe for sign extend.
> > At this point you are using 
> > ```
> > if(Imm >= maxUIntN(NarrowVT.getSizeInBits()))
> >   break;
> > ```
> > to check if the immediate fits in the not yet extended range.
> > In the SIGN_EXTEND case you can do this by using 
> > ```
> > if (Op1.getOpcode() == ISD::SIGN_EXTEND && Imm >= maxUIntN(NarrowVT.getSizeInBits() - 1))
> >       break;
> > ```
> > When you subtract 1 from the range you know that the sign bit on whatever we extend is going to be zero (because it is for the constant and we are doing an AND) so the sign extend becomes a zero extend and everything is fine.
> > 
> > I'm not 100% about this idea but it might work.
> I think this may be correct, but I can't envision a situation where we sign-extend a value and then AND it with a narrower value. The SDAG should always convert such "don't care" extends to `ANY_EXTEND`s.
Fair enough. Don't worry about it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152911