[PATCH] D39536: [PowerPC] Eliminate redundant register copys after register allocation

[Zaara Syeda via Phabricator via llvm-commits]

syzaara added inline comments.


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:12
+// after the register allocation.
+// So far, we eliminate the following two types of redundant register copys.
+//
----------------
s/copys/copies


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:87
+// source and destination operands in SrcOperand and DstOperand.
+// When IsKill is true, this method returns true only if the src operand
+// has kill flag set.
----------------
Is this an old comment you forgot to remove? I don't see anything related to IsKill in this function.


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:121
+    for (auto &MO : MBBI->operands()) {
+      if (MO.isReg() && !MO.isDef() &&
+          TRI->isSuperOrSubRegisterEq(MO.getReg(), Reg))
----------------
I think this flow would be more clear if written like:

```
      if (MO.isReg() && TRI->isSuperOrSubRegisterEq(MO.getReg(), Reg)) {
          if (MO.isUse())
              return false;
          else if(MO.isDef() ||  (MO.isRegMask() && MO.clobbersPhysReg(Reg)))
              SrcKilled = true; // we may still have use of Reg in this MI
      }
```


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:134
+
+  // We look for Reg in the seccessor BB liveins.
+  for (auto &SuccMBB : MBB->successors())
----------------
s/seccessor/successor


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:173
+      MachineOperand &DefMO = MBBRI->getOperand(Idx);
+      if (DefMO.isReg() && DefMO.getReg() == SrcReg &&
+          !DefMO.isTied() && !DefMO.isImplicit() &&
----------------
Since findRegisterDefOperandIdx already checks for SrcReg, do we need to check DefMO.getReg() == SrcReg here?


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:196
+        if (TRI->isSuperOrSubRegisterEq(MO.getReg(), DstReg)) {
+          if (!MO.isDef())
+            IsDstUsed = true;
----------------
MO.isUse() would be more clear


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:210
+          //   mr r3, r4           (erase mr)
+          if (MO.getReg() == SrcReg)
+            TmpOperands.push_back(&MO);
----------------
It seems confusing that we check for registers being equal with isSuperOrSubRegisterEq and then again with if (MO.getReg() == SrcReg).
Can we rewrite to something like:

```
if (MO.getReg() == SrcReg)
   TmpOperands.push_back(&MO);
else if (TRI->isSuperOrSubRegisterEq(MO.getReg(), SrcReg)) {
   IsClobbered = true;
   return nullptr;
}
```



================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:314
+                MO->setReg(DstReg);
+              CurrentInstrErased = true;
+              break;
----------------
This is repeated below for the second case on line 443
However, the second case also sets Simplified = CurrentInstrErased which doesn't seem to be set here.
Can this be rewritten in a way that we only have to do this in one place for both cases?


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:339
+          // predecessors not to increase the number of total instructions.
+          if (MBB.pred_size() > 2)
+            continue;
----------------
Can we put these checks of pred_size/DstLive/control flow for which we just continue into a separate function and do something like:

```
if (!controlFlowSupported())
    continue;
```


https://reviews.llvm.org/D39536