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

[Geoff Berry via Phabricator via llvm-commits]

gberry marked 3 inline comments as done.
gberry added a comment.

I've fixed some of the comments and have questions about the others.
I also re-based and fixed an issue with instructions that have wider register implicit uses that are implicitly tied to other operands.  The added code is the check in forwardUses() that references AMDGPU in the preceding comment.



================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:188
+                    "Machine Copy Propagation Pre-Register Rewrite Pass", false,
+                    false)
+
----------------
qcolombet wrote:
> 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? 
I took a look at this, and the main problem that comes up when doing it as one pass is the fact that you wouldn't have separate pass IDs that can be used to disable the later run of this pass but not the earlier one (e.g. as is done by NVPTX and WebAssembly targets).  I also think it might make it hard to run the pass in isolation via llc.  If you think these issues are surmountable with the single pass approach, let me know and I'll take another look.


================
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) {
----------------
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'.


================
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;
----------------
qcolombet wrote:
> Put /*ForClobber*/ (or the updated name) in from of false.
I refactored this to put the 'FullReg' parameter in the function name instead to be more clear.


================
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
----------------
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.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:590
+
+      forwardUses(*MI);
+
----------------
inouehrs wrote:
> Do we need to execute forwardUses after virtual register rewriting again?
There are additional forwarding opportunities exposed later in the pipeline by e.g. tail-merging and tail-duplication that are caught by this second run. 


https://reviews.llvm.org/D30751