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

[Hiroshi Inoue via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:121
+    for (auto &MO : MBBI->operands()) {
+      if (MO.isReg() && !MO.isDef() &&
+          TRI->isSuperOrSubRegisterEq(MO.getReg(), Reg))
----------------
syzaara wrote:
> 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
>       }
> ```
They are calling different methods `isSuperOrSubRegisterEq` and `isSubRegisterEq`, but I gave a try to make the control flow clearer. 


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:173
+      MachineOperand &DefMO = MBBRI->getOperand(Idx);
+      if (DefMO.isReg() && DefMO.getReg() == SrcReg &&
+          !DefMO.isTied() && !DefMO.isImplicit() &&
----------------
syzaara wrote:
> Since findRegisterDefOperandIdx already checks for SrcReg, do we need to check DefMO.getReg() == SrcReg here?
Yes, it intends to confirm that it is not sub/super register.


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:210
+          //   mr r3, r4           (erase mr)
+          if (MO.getReg() == SrcReg)
+            TmpOperands.push_back(&MO);
----------------
syzaara wrote:
> 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;
> }
> ```
> 
Fixed.
Originally, I intended to make the if-statement parallel as
- isSuperOrSubRegisterEq for DstReg
- isSuperOrSubRegisterEq for SrcReg




================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:285
+          // Def for the source of the register copy is found in the BB.
+          if (DefMI) {
+            if (!IsDstUsed) {
----------------
syzaara wrote:
> I'm finding this difficult to follow since there is a lot happening in the loop. Can we extract out more functionality into functions? From what I understand, maybe we can do something like:
> 
> ```
> if (DefMI)
>    Simplified |= eliminateIntraBBCopy
> else if (controlFlowSupported())
>    Simplified |= elimanteInterBBCopy
> ```
I hope it becomes easier to follow now.


https://reviews.llvm.org/D39536