[PATCH] D69168: [PowerPC] Fold redundant load immediates of zero and delete if possible

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1392
 
+// This is a modified version of FoldImmediate where the DefMI is not deleted
+// Its purpose is to defer the decision to delete to the caller of this function
----------------
End with period. 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1397
+                                              MachineInstr &DefMI, unsigned Reg,
+                                              MachineRegisterInfo *MRI) const {
+  // For some instructions, it is legal to fold ZERO into the RA register field.
----------------
Is `MRI` needed in this function/is it being used?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1418
+
+  unsigned UseIdx;
+  for (UseIdx = 0; UseIdx < UseMI.getNumOperands(); ++UseIdx)
----------------
Can we add a comment here describing the following?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1453
+              PPC::ZERO8 : PPC::ZERO;
+  }
+  UseMI.getOperand(UseIdx).setReg(ZeroReg);
----------------
nit: Add a space here maybe for better readability. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:321
+        // recognize 0 as the number and not the register and replace this load
+        // as an operand with PPC::ZERO(8) then remove the load if it is no
+        // needed.
----------------
`if it is not needed`


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:323
+        // needed.
+        if (!MI.getOperand(1).isImm())
+          break;
----------------
Correct me if I am wrong, but isn't this being checked inside `FoldImmediateWithoutDelete`?
```
 if (!DefMI.getOperand(1).isImm())
    return false;
  if (DefMI.getOperand(1).getImm() != 0)
    return false;
```
Do we need to check this in both places?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69168