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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added inline comments.
This revision now requires changes to proceed.


================
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) {
----------------
gberry wrote:
> qcolombet wrote:
> > I found the name of the function confusing.
> > It takes MIs but mention only reg classes.
> How about isForwardableRegClassCopy?  I also renamed the second parameter to 'UseI'.
Works with better comment on top. See next comment.


================
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
----------------
gberry wrote:
> qcolombet wrote:
> > gberry wrote:
> > > qcolombet wrote:
> > > > That's invalid per the MachineVerifier
> > > Can you elaborate on this?  I'm not sure what you are referring to as being invalid.
> > Sorry. Physical registers don't have subreg indices. The machine verifier checks that.
> In this case, the MOUse may not be a physical register.  We are replacing a possibly virtual reg (MOUse) with a physical reg (NewUseReg), so we need to make sure to translate the subreg on MOUse.  I've added a comment to hopefully clarify this a bit.
Ah right, I missed the *New* in the name of the check.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:370
+      DstRC = TRI->getMinimalPhysRegClass(DstReg);
+    }
+
----------------
No bracket


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:395
+  // so we have reduced the number of cross-class COPYs and potentially
+  // introduced a no COPY that can be removed.
+  if (!UseI.isCopy())
----------------
I think this example would do great on the comment of the function itself with a note that Copy is the first copy and UseI the second one. Of course, we need to document the non-copy UseI case as well :)


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:399
+
+  return !isCross(UseI.getOperand(0), CopySrc);
+}
----------------
I'd add an assert that UseI.getOperand(1) == DstReg


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:403
+void MachineCopyPropagation::updateForwardedCopyLiveInterval(
+    unsigned CopyDstReg, unsigned CopySrcReg, const MachineInstr &UseMI) {
+  SmallVector<MachineInstr *, 4> DeadInsts;
----------------
Add a comment on what are the relation between CopyDstReg, CopySrcReg and UseMI and assert on that.
E.g., UseMI is a copy that uses CopyDstReg. CopyDstReg is going to be replaced by CopySrcReg.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:404
+    unsigned CopyDstReg, unsigned CopySrcReg, const MachineInstr &UseMI) {
+  SmallVector<MachineInstr *, 4> DeadInsts;
+  LiveInterval &LI = LIS->getInterval(CopyDstReg);
----------------
Move that closer to its first use.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:405
+  SmallVector<MachineInstr *, 4> DeadInsts;
+  LiveInterval &LI = LIS->getInterval(CopyDstReg);
+
----------------
Unless I am missing something, I don't see the extend to use call for CopySrcReg.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:432
+
+void MachineCopyPropagation::forwardUses(MachineInstr &MI) {
+  if (AvailCopyMap.empty())
----------------
Add a comment on what this is doing.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:443
+
+    unsigned UseReg = MOUse.getReg();
+
----------------
I would add:
if (!UseReg) continue;


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:452
+      // restrict the types of instructions we forward physical regs into.
+      continue;
+
----------------
I would avoid forwarding on physical register period.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:524
+    if (hasImplicitOverlap(MOUse, MI))
+      continue;
+
----------------
We don't need this check for virtual reg, right?


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:571
+      }
+    }
+
----------------
It would help the readability if there were helper function for the different regclass/register fixing for the respective physreg and virtureg cases.

E.g., a helper with if NewUseReg is a physreg then block line 485 else block line 541.
Then the code would look like:
// Check if replacing is possible between MOUse and NewUseReg
[...]
isForwardXXX
[...]
// Adapt NewUseReg to whatever constraints are carried by MOUse
fix(NewUseReg, NewUseSubReg); // <--- This call your helper function


================
Comment at: test/CodeGen/AArch64/flags-multiuse.ll:1
-; RUN: llc -mtriple=aarch64-none-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -verify-machineinstrs -o - %s | FileCheck %s
+; RUN: llc -mtriple=aarch64-none-linux-gnu -aarch64-enable-atomic-cfg-tidy=0 -disable-post-ra -verify-machineinstrs -o - %s | FileCheck %s
 
----------------
Why do we need to change the run line here?


https://reviews.llvm.org/D30751