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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15475
+    if ((Op1.getOpcode() != ISD::ZERO_EXTEND &&
+         Op1.getOpcode() != ISD::ANY_EXTEND) ||
+        !isa<ConstantSDNode>(Op2))
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15482
+        NarrowOp.getOpcode() != ISD::ROTR &&
+        NarrowOp.getOpcode() != ISD::FSHL && NarrowOp.getOpcode() != ISD::FSHR)
+      break;
----------------
stefanp wrote:
> nit:
> ```
> NarrowOp.getOpcode()
> ```
> is used a lot in this if statement. You may want to pull it out.
> ```
> unsigned NarrowOpcode = NarrowOp.getOpcode();
> if (NarrowOpcode != ISD::SHL && ...
> ```
Makes sense. Not sure why I didn't do this.


================
Comment at: llvm/test/CodeGen/PowerPC/and-extend-combine.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc < %s -mtriple=powerpc64le-unknown-unknown -ppc-asm-full-reg-names \
+; RUN:   -mcpu=pwr8 -verify-machineinstrs | FileCheck %s
----------------
amyk wrote:
> Is an AIX run line also necessary?
I don't think there is anything to be gained by adding it. The code is completely independent of the platform or even subtarget.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152911