[PATCH] D67794: [MachineCopyPropagation] Extend MCP to do trivial copy backward propagation

[Kai Luo via Phabricator via llvm-commits]

lkail marked an inline comment as done.
lkail added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:212
+    return AvailCopy;
+  }
+
----------------
qcolombet wrote:
> qcolombet wrote:
> > It seems to me that we could just reuse directly findAvailCopy and thus eliminate findCopyDefViaUnit.
> > The only difference would be the that we use a different iterator for the. forward and the backward case.
> Spoke too soon. I see the difference in findCopyDefViaUnit:
> We check that the source of the copy is used only once, then that the reg unit defined by this copy is available.
> 
> I still need to go through the rest of that patch, but I would expect that we could do more code reuse if we were to drop from the list of available copies anything that is read or clobbered between the use and the definition. I.e., the case were more than one copy use the same source (which is basically what `DefRegs.size() != 1` represents) wouldn't be kept in the `Copies` variable to begin with.
> 
> Anyway, not something that must be fixed here. We can refactor later.
> Let me go through the whole patch first.
I agree with that there are still some redundancies in current code. I think `findAvailBackwardCopy` can be combined into `findAvailCopy` with an additional flag to indicate if its forward or backward and `reverseIterator` in `findAvailBackwardCopy` can be replaced with `Iterator`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67794/new/

https://reviews.llvm.org/D67794