[PATCH] D30113: [AArch64] Extend AArch64RedundantCopyElimination to do simple copy propagation.

[Geoff Berry via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:118
+  // Registers that are known to contain zeros at the start of MBB.
+  SmallVector<unsigned, 4> TargetRegs;
+  // Registers clobbered in PredMBB between CompBr instruction and current
----------------
MatzeB wrote:
> Use `MCPhysReg` instead of `unsigned`, possibly at a few more places below as well.
> 
> Why is this called TargetRegs? How about calling this `ZeroRegs` or `NullRegs` if you don't want it confused with xzr/wzr?
I changed all of these except for the SmallSetVector element type for MCPhysReg.  That one can't be changed without also adding a DenseMapInfo<uint16_t> specialization, which doesn't seem worth doing for this change to me.

As for 'TargetRegs': Yeah, I considered renaming this variable as well.  I went with 'KnownZeroRegs' in the latest version.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:169-171
+  // We've not found a known zero register, time to bail out.
+  if (TargetRegs.empty())
     return false;
----------------
MatzeB wrote:
> Should this have an additional check for situations in which all target regs are clobbered (similar to the one a few lines earlier?)
I don't think so.  The clobbered reg tracking is for regs that are clobbered as we move backwards from the e.g. CBZ instruction in PredBB.  At this point in the code we start looking forward at the start of MBB and process clobbers by removing the known zero regs from the list instead.


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:3-12
+--- |
+  define void @test1() { ret void }
+  define void @test2() { ret void }
+  define void @test3() { ret void }
+  define void @test4() { ret void }
+  define void @test5() { ret void }
+  define void @test6() { ret void }
----------------
MatzeB wrote:
> I recently found out that you can even leave out the whole IR string literal and MIRParser will create dummy functions for you, that should work here.
> 
> (Unfortunately as soon as you need to define other IR elements like globals or declaring external functions you need the IR block and MIRParser won't create dummy function for you right now) 
Good to know.


================
Comment at: test/CodeGen/AArch64/machine-copy-remove.mir:273
+
+...
----------------
qcolombet wrote:
> Could you add a positive and negative test case when the copy we want to eliminate is at the beginning of a block with two or more predecessors.
I added a test (test9) to make sure we aren't removing a COPY from a block with multiple predecessors.  I'm not sure what you mean by a positive test in this case though.


https://reviews.llvm.org/D30113