[PATCH] D52374: [MachineCopyPropagation] Reimplement CopyTracker in terms of register units

[Justin Bogner via Phabricator via llvm-commits]

bogner marked 2 inline comments as done.
bogner added inline comments.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:133-136
+  bool hasAvailableCopies() {
+    // TODO: This is conservatively correct, but is it useful?
+    return !Copies.empty();
   }
----------------
MatzeB wrote:
> This feels odd now, as in the new code you say `Avail = false` instead of removing elements from the `Copies` set... Would it make sense to maintain a counter of entries with `Avail == true` for this?
It's not really worth the bookkeeping to keep track of the number of available copies, but this is used to shortcut out of forwardUses and it's measurably slower if we don't do it at all.

I've renamed this to hasAnyCopies so it's more obvious what it actually checks.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:148-155
+  MachineInstr *findCopy(unsigned Reg, const TargetRegisterInfo &TRI,
+                         bool MustBeAvailable = false) {
+    MCRegUnitIterator RUI(Reg, &TRI);
+    MachineInstr *MI = findCopyForUnit(*RUI, TRI, MustBeAvailable);
+    if (!MI || !TRI.isSubRegisterEq(MI->getOperand(0).getReg(), Reg))
+      return nullptr;
+    return MI;
----------------
MatzeB wrote:
> Is it correct to only check the first register unit here? Isn't it possible that only some of the units gots clobbered in the meantime, while the first one still makes it seem like the copy is available?
Yes, as discussed offline this is correct. When we clobber units we end up marking registers unavailable based on the original instruction's defs (not just the unit itself), so we won't hit any problems here.


https://reviews.llvm.org/D52374