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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think that the main loop definitely needs to be re-written and made more readable and easier to follow. To me, it seems like it does too much. It spans nearly 250 lines and does a whole bunch of analyses and transformations.
Could you extract functionality into easy-to-read functions that perform individual things so that the flow is easier to follow?

Also, would it be appropriate to make this target-independent and do it perhaps in `MachineCopyPropagation`? Perhaps Geoff can comment on that since he's done something similar that he is still working on improvements for in order to finally commit it.



================
Comment at: lib/Target/PowerPC/PPCPostRAPeephole.cpp:61
+  // Perform peepholes.
+  bool eliminateRedundantRegCopy(void);
+
----------------
If we're adding a new peephole, it likely won't only be used for this one purpose. A more general name might be in order.


================
Comment at: lib/Target/PowerPC/PPCPostRAPeephole.cpp:100
+    DstOperand = &MI.getOperand(0);
+    // We return the last operand as src to access kill flag.
+    SrcOperand = &MI.getOperand(2);
----------------
This seems a bit fragile. I'm not sure there's any hard guarantee that the flag will be on the last source operand.


================
Comment at: lib/Target/PowerPC/PPCPostRAPeephole.cpp:142
+            // target for our optimization.
+            for (auto DefMO: MBBRI->defs())
+              if (TRI->isSuperOrSubRegisterEq(DefMO.getReg(), SrcReg)) {
----------------
These loops as well as a check for whether the instruction is a call seem a little odd. Why is it inadequate to use `MachineInstr::readsRegister()` and `MachineInstr::modifiesRegister()` for these purposes. Given the TRI, they should return the correct value for any instruction (including calls).


https://reviews.llvm.org/D39536