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

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

The remaining comments are minor nits that can be addressed on the commit. Please don't forget the follow-up I requested at https://reviews.llvm.org/D40554#inline-1239933.
Other than those, LGTM.

Thanks for your patience with this patch.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5199-5203
+  if ((Opcode == PPC::LBZU || Opcode == PPC::LBZUX || Opcode == PPC::LBZU8 ||
+       Opcode == PPC::LBZUX8 || Opcode == PPC::LHZU || Opcode == PPC::LHZUX ||
+       Opcode == PPC::LHZU8 || Opcode == PPC::LHZUX8 || Opcode == PPC::LWZU ||
+       Opcode == PPC::LWZUX || Opcode == PPC::LWZU8 || Opcode == PPC::LWZUX8) &&
+      MI->getOperand(0).getReg() == Reg)
----------------
Since we repeat this code (the subword part), maybe it would be cleaner to extract it into a static local function - something like:
```
static bool isOpZeroOfSubwordPreincLoad(const MachineInstr &MI)
```
Of course, since you still need the LWZU variants, it may not be worth it.
This is a minor stylistic comment that shouldn't delay approval.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5284
+      // If this is a copy from another register, we recursively check source.
+      auto SrcExt = isSignOrZeroExtended(SrcReg, BinOpDepth, MRI);
+      return std::pair<bool, bool>(SrcExt.first || IsSExt,
----------------
I am a little unclear on the use of `BinOpDepth` in this function. It seems very inconsistent. Wouldn't it make sense to always add one for the recursive call as well as to check whether it has been exceeded prior to the call? We seem to do that when checking for reg+reg instructions but not for reg+imm instructions. Is that because a single recursive call avoids exponential recursion whereas the two calls for reg+reg instructions don't? If so, can you please add a comment to that end at the top of the function?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5308-5313
+    if (SrcReg != PPC::X3) {
+      // If this is a copy from another register, we recursively check source.
+      auto SrcExt = isSignOrZeroExtended(SrcReg, BinOpDepth, MRI);
+      return std::pair<bool, bool>(SrcExt.first || IsSExt,
+                                   SrcExt.second || IsZExt);
+    }
----------------
Since the comment above this block describes the code below, maybe just move this block above the comment.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5318
+        MachineBasicBlock::const_instr_iterator(MI);
+    if (II == MBB->instr_begin() ||
+        (--II)->getOpcode() != PPC::ADJCALLSTACKUP)
----------------
Super minor nit: since we construct the same pair in 3 different early exits as well as at the end, maybe just construct it once and return it in each location (lines 5320, 5324, 5329, 5340).
There is no functional or performance difference, but it is very clear that what we are returning is the exact same thing and couldn't have changed.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5327
+    const Function *CalleeFn =
+        dyn_cast<Function>(CallMI.getOperand(0).getGlobal());
+    if (!CalleeFn)
----------------
I think it may be possible for `CallMI.getOperand(0).getGlobal()` to return `null` under some odd circumstances. It would probably be safer to use `dyn_cast_if_present` here rather than `dyn_cast`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5339
+
+    // If this is a copy from another register, we recursively check source.
+    return std::pair<bool, bool>(IsSExt, IsZExt);
----------------
This comment is out of date now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D40554