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

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked 8 inline comments as done.
inouehrs added inline comments.


================
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);
   }
----------------
nemanjai wrote:
> 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.
I changed the names of variable and constant to indicate they shows the depth of binary operators.


================
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:
----------------
nemanjai wrote:
> Why does the register input matter for `and`-immediate?
Good catch. I fixed.


================
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());
 
----------------
nemanjai wrote:
> Comment with the assert.
I added comment, but I feel this assert is not necessary.


================
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,
----------------
nemanjai wrote:
> 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.
I agree that the single query approach is cleaner. But I am afraid that it will increase the cost of compilation slightly since it always checks for both sign- and zero-extensions. Do you think the potential increase in compilation cost does not matter too much here?


https://reviews.llvm.org/D40554