[PATCH] D89855: [PowerPC] Extend folding RLWINM + RLWINM to post-RA.

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added a comment.

Some nits.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3192
 
-bool PPCInstrInfo::combineRLWINM(MachineInstr &MI,
-                                 MachineInstr **ToErase) const {
+// This function tries to combine two RLWINMs. We not only perform such
+// optimization in SSA, but also after RA, since some RLWINM is generated after
----------------
Comments need to be updated.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3196
+bool PPCInstrInfo::simplifyRotateAndMaskInstr(MachineInstr &MI,
+                                              MachineInstr **ToErase) const {
+  unsigned MIOpCode = MI.getOpcode();
----------------
How about use the MachineInstr *&ToErase to avoid the null check and default arguments ?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3203
+  Register FoldingReg = MI.getOperand(1).getReg();
+  MachineInstr *SrcMI;
+  // If we're in SSA.
----------------
It is preferred to initialize the stack variable.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3204
+  MachineInstr *SrcMI;
+  // If we're in SSA.
+  if (MRI->isSSA()) {
----------------
Remove such kind of comments as the code is self explained.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3209
+    SrcMI = MRI->getVRegDef(FoldingReg);
+    if (!SrcMI)
+      return false;
----------------
Common line 3209 and 3217 then, add a check in 3220


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3228
+                  MIOpCode == PPC::ANDI8_rec);
+  if (Is64Bit != IsSrc64Bit)
     return false;
----------------
A better code style to avoid the duplicate check of the SrcOpCode.
```
switch (SrcOpCode) {
case RLWINM8
case RLWINM8_rec:
    Is64Bit = true;
    break;
case ...
}
```



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3347
+        (MRI->isSSA() && MRI->use_nodbg_empty(FoldingReg))) {
+      *ToErase = SrcMI;
+      LLVM_DEBUG(dbgs() << "Delete dead instruction: ");
----------------
null  check?


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:40
           "Number of folding frame offset by using r+r in pre-emit peephole");
+STATISTIC(NumCombineRLWINMInPreEmit,
+          "Number of combining RLWINM with ANDI/RLWINM in pre-emit peephole");
----------------
Change the name as it is extended to all rotate combining.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:422
+            NumCombineRLWINMInPreEmit++;
+            if (ToErase) {
+              InstrsToErase.push_back(ToErase);
----------------
No {} if there is only one statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89855