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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Sorry, I had some comments that I haven't submitted because I had not gotten through the entire patch. I am not sure if they all still apply but thought I'd submit them before restarting the review.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5091-5094
+// This function checks the machine instruction that defines the input regsiter
+// Reg. If that machine instruction always outputs a value by sign-extending a
+// 32 bit value, i.e. 0 to 31-th bits are same as 32-th bit then this function
+// will return true.
----------------
I think all explanations of what sign-extending means should be removed and you should just note that this is a test explicitly for sign extension from 32 to 64 bits.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5105
+  // Opcodes included in this set always return sign-extended value.
+  static const DenseSet<int> OpcodesSExt32To64{
+      PPC::LI,           PPC::LI8,       PPC::LIS,          PPC::LIS8,
----------------
Rather than having tables of these opcodes in the C++ code, we should probably just add a couple of flags in our .td files for the two types of extension. Perhaps `isSExt32To64, isZExt32To64`. Then we can simply query that on the `MI`.

Of course, the ones below this set probably have to stay because they have further constraints.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5119
+  int Opcode = MI->getOpcode();
+  if (OpcodesSExt32To64.find(Opcode) != OpcodesSExt32To64.end())
     return true;
----------------
Is there no `DenseSet<>::count()`?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5288
       if (SrcReg == PPC::X3) {
-        const MachineBasicBlock *MBB = MI.getParent();
+        const MachineBasicBlock *MBB = MI->getParent();
         MachineBasicBlock::const_instr_iterator II =
----------------
This block ends up getting very deeply nested and hard to follow. Can we flip the conditions and do early exits? Something like:
```
if (SrcReg != PPC::X3)
  return ...
...
if previous instr is not the stack adjust or the one before is not a call...
  return ...
```
and so on.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5313
 
     // If this is a copy from another register, we recursively check source.
+    auto SrcExt = isSignOrZeroExtended(SrcReg, BinOpDepth, MRI);
----------------
This code might need to move up if we are flipping the previous condition.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:602-603
 
-  /// Return true if the output of the instruction is always a sign-extended,
+  /// Return true if the register is always a sign-extended,
   /// i.e. 0 to 31-th bits are same as 32-th bit.
+  bool isSignExtended(const unsigned Reg,
----------------
```
// Return true if the register is sign-extended from 32 to 64 bits.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:609-610
 
-  /// Return true if the output of the instruction is always zero-extended,
+  /// Return true if the register is always zero-extended,
   /// i.e. 0 to 31-th bits are all zeros
+  bool isZeroExtended(const unsigned Reg,
----------------
```
// Return true if the register is zero-extended from 32 to 64 bits.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D40554