[PATCH] D30751: [MachineCopyPropagation] Extend pass to do COPY source forwarding

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:130
+            .set(MachineFunctionProperties::Property::TracksLiveness);
+      else
+        return MachineFunctionProperties().set(
----------------
No else.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:188
+                    "Machine Copy Propagation Pre-Register Rewrite Pass", false,
+                    false)
+
----------------
I am wondering if we want to obfuscate that this is really just the same pass with different parameters. I get that the dependencies are also different for the initialization process, but usually we just go with the most constraining one.

Is it worth doing differently here? 


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:268
+///
+/// The 'ForClobber' parameter specifies whether we want the full physical
+/// register assigned to the virtual register ignoring subregs or not.  If we
----------------
What about FullReg (or getSubReg if you want to invert) instead of ForClobber?


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:296
+  Reg = TargetRegisterInfo::isVirtualRegister(Reg) ? VRM->getPhys(Reg) : Reg;
+  unsigned SubReg = Opnd.getSubReg();
+  if (SubReg && !(ForClobber && PreRegRewrite && NoSubRegLiveness))
----------------
physregs don't have subreg.
Thus, you could make the code more readable with:
if (!TargetRegisterInfo::isVirtualRegister(Reg))
  return Reg;
assert(PreRegRewrite);

// Then test over sub reg


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:340
+// Only forward cross-class COPYs into other reversed cross-class COPYs.
+bool MachineCopyPropagation::isForwardableRegClass(MachineInstr &Copy,
+                                                   MachineInstr &I) {
----------------
I found the name of the function confusing.
It takes MIs but mention only reg classes.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:437
+    // Don't forward COPYs that are already NOPs due to register assignment.
+    if (getPhysReg(CopyDst, false) == getPhysReg(CopySrc, false))
+      continue;
----------------
Put /*ForClobber*/ (or the updated name) in from of false.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:455
+      if (MOUse.getSubReg())
+        NewUseReg = TRI->getSubReg(NewUseReg, MOUse.getSubReg());
+      // If the original use subreg isn't valid on the new src reg, we can't
----------------
That's invalid per the MachineVerifier


https://reviews.llvm.org/D30751