[PATCH] D40554: [PowerPC] Fix bugs in sign-/zero-extension elimination

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5180
 
-  // There are other instructions that clear higher 32-bits.
-  if (Opcode == PPC::CNTLZW || Opcode == PPC::CNTLZW_rec ||
-      Opcode == PPC::CNTTZW || Opcode == PPC::CNTTZW_rec ||
-      Opcode == PPC::CNTLZW8 || Opcode == PPC::CNTTZW8 ||
-      Opcode == PPC::CNTLZD || Opcode == PPC::CNTLZD_rec ||
-      Opcode == PPC::CNTTZD || Opcode == PPC::CNTTZD_rec ||
-      Opcode == PPC::POPCNTD || Opcode == PPC::POPCNTW || Opcode == PPC::SLW ||
-      Opcode == PPC::SLW_rec || Opcode == PPC::SRW || Opcode == PPC::SRW_rec ||
-      Opcode == PPC::SLW8 || Opcode == PPC::SRW8 || Opcode == PPC::SLWI ||
-      Opcode == PPC::SLWI_rec || Opcode == PPC::SRWI ||
-      Opcode == PPC::SRWI_rec || Opcode == PPC::LWZ || Opcode == PPC::LWZX ||
-      Opcode == PPC::LWZU || Opcode == PPC::LWZUX || Opcode == PPC::LWBRX ||
-      Opcode == PPC::LHBRX || Opcode == PPC::LHZ || Opcode == PPC::LHZX ||
-      Opcode == PPC::LHZU || Opcode == PPC::LHZUX || Opcode == PPC::LBZ ||
-      Opcode == PPC::LBZX || Opcode == PPC::LBZU || Opcode == PPC::LBZUX ||
-      Opcode == PPC::LWZ8 || Opcode == PPC::LWZX8 || Opcode == PPC::LWZU8 ||
-      Opcode == PPC::LWZUX8 || Opcode == PPC::LWBRX8 || Opcode == PPC::LHBRX8 ||
-      Opcode == PPC::LHZ8 || Opcode == PPC::LHZX8 || Opcode == PPC::LHZU8 ||
-      Opcode == PPC::LHZUX8 || Opcode == PPC::LBZ8 || Opcode == PPC::LBZX8 ||
-      Opcode == PPC::LBZU8 || Opcode == PPC::LBZUX8 ||
-      Opcode == PPC::ANDI_rec || Opcode == PPC::ANDIS_rec ||
-      Opcode == PPC::ROTRWI || Opcode == PPC::ROTRWI_rec ||
-      Opcode == PPC::EXTLWI || Opcode == PPC::EXTLWI_rec ||
-      Opcode == PPC::MFVSRWZ)
+  const PPCInstrInfo *TII =
+      MI->getMF()->getSubtarget<PPCSubtarget>().getInstrInfo();
----------------
amyk wrote:
> Question: This is something super minor, but is it better to put the check for `isZExt32To64()` earlier in the function? 
> 
> I know it may not matter, but I am just wondering in case it may be better to do this prior to all of the different opcode checks, and since the `definedBySignExtendingOp` also put it's `isSExt32To64()` pretty early in the function.
> 
> In any case, this is just a question so if it's not applicable, you can feel free to disregard this.
> Question: This is something super minor, but is it better to put the check for `isZExt32To64()` earlier in the function? 
> 
> I know it may not matter, but I am just wondering in case it may be better to do this prior to all of the different opcode checks, and since the `definedBySignExtendingOp` also put it's `isSExt32To64()` pretty early in the function.
> 
> In any case, this is just a question so if it's not applicable, you can feel free to disregard this.

I don't think it matters in this case. For this check we need to get `TII` so maybe it's better to check other things first that don't require the instruction info? That may save us from having to get the additional information in a limited number of cases. On the other hand we are just checking a single flag for a large number of instructions.
Anyway, I really don't feel strongly about it one way or another. I'm going to move it up just because it is nicer style to have the two functions a little more similar. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D40554