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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:3048
+  int Opcode = MI->getOpcode();
   if (Opcode == PPC::LI     || Opcode == PPC::LI8     ||
       Opcode == PPC::LIS    || Opcode == PPC::LIS8    ||
----------------
I think the number of opcodes here is large enough to warrant storing them in a data structure with efficient searching such as a dense set. In addition to readability at the point where you're querying this, it can be very clear from the name what the data structure contains.

For example, if you have a container called `OpcodesZExt32To64`, it is very clear what it is for and any future code that needs such functionality can easily access that container rather than re-writing this whole list.

Of course, the same applies to the similar list for zero extending ops.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:3249
     // If this is a copy from another register, we recursively check source.
-    if (!TargetRegisterInfo::isVirtualRegister(SrcReg))
-      return false;
-    const MachineInstr *SrcMI = MRI->getVRegDef(SrcReg);
-    if (SrcMI != NULL)
-      return isSignOrZeroExtended(*SrcMI, SignExt, Depth);
-
-    return false;
+    return isSignOrZeroExtended(SrcReg, SignExt, Depth, MRI);
   }
----------------
Seems a bit misleading to have a depth parameter that is not actually respected if we're looking through a chain of copies. Although of course, deep chains of copies shouldn't happen in practice.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:3254
+  // So, we track the operand register as we do for register copy.
   case PPC::ANDIo:
   case PPC::ORI:
----------------
Why does the register input matter for `and`-immediate?


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:3261
+    unsigned SrcReg = MI->getOperand(1).getReg();
+    return isSignOrZeroExtended(SrcReg, SignExt, Depth, MRI);
+  }
----------------
This recursive call as well as the next also don't check the depth, right? I realize that these immediate-form binary logical ops won't be chained, but it's still good to add a comment explaining we don't need to limit the depth on such ops.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:3317
 
-    assert(MI.getOperand(1).isReg() && MI.getOperand(2).isReg());
-
-    unsigned SrcReg1 = MI.getOperand(1).getReg();
-    unsigned SrcReg2 = MI.getOperand(2).getReg();
+    assert(MI->getOperand(1).isReg() && MI->getOperand(2).isReg());
 
----------------
Comment with the assert.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.h:343
 
-  bool isSignOrZeroExtended(const MachineInstr &MI, bool SignExt,
-                            const unsigned PhiDepth) const;
+  bool isSignOrZeroExtended(const unsigned Reg, bool SignExt,
+                            const unsigned PhiDepth,
----------------
I still think this setup with separate queries for sign/zero/any extend seems unnecessarily complex. Seems like it would be clearer if we had an enum for the extension type (sign, zero, none) and we just have a single query that will return this. Therefore no need for Boolean parameters, no need for multiple functions, etc.


https://reviews.llvm.org/D40554