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

[Geoff Berry via Phabricator via llvm-commits]

gberry marked 12 inline comments as done.
gberry added inline comments.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:405
+  SmallVector<MachineInstr *, 4> DeadInsts;
+  LiveInterval &LI = LIS->getInterval(CopyDstReg);
+
----------------
qcolombet wrote:
> Unless I am missing something, I don't see the extend to use call for CopySrcReg.
That was being done before the call to this function.  I've moved it into this function in my latest revision.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:452
+      // restrict the types of instructions we forward physical regs into.
+      continue;
+
----------------
qcolombet wrote:
> I would avoid forwarding on physical register period.
I have seen some benefit to forwarding physical registers, mostly in cases where block boundaries are changed between RA time and the second run of this pass.  This mostly seems to happen when tail merge/tail duplication changes which uses COPYs are exposed to in the same block.  This pass won't remove any physical register writes, so this seems relatively safe, other than the caveat in the comment above.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:524
+    if (hasImplicitOverlap(MOUse, MI))
+      continue;
+
----------------
qcolombet wrote:
> We don't need this check for virtual reg, right?
No, I don't think so.  I added an early exit for this case to hasImplicitOverlap.


================
Comment at: lib/CodeGen/MachineCopyPropagation.cpp:571
+      }
+    }
+
----------------
qcolombet wrote:
> 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
I've factored out quite a bit of code from this function, let me know what you think.


================
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
 
----------------
qcolombet wrote:
> Why do we need to change the run line here?
Turning off the post-RA scheduler kept the checked instructions in the same order.  I've just re-arranged the checks now.


https://reviews.llvm.org/D30751