This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Extend AArch64RedundantCopyElimination to do simple copy propagation.
ClosedPublic

Authored by gberry on Feb 17 2017, 12:37 PM.

Details

Summary

Extend AArch64RedundantCopyElimination to catch cases where the register
that is known to be zero is COPY'd in the predecessor block. Before
this change, this pass would catch cases like:

    CBZW %W0, <BB#1>
BB#1:
    %W0 = COPY %WZR // removed

After this change, cases like the one below are also caught:

    %W0 = COPY %W1
    CBZW %W1, <BB#1>
BB#1:
    %W0 = COPY %WZR // removed

This change results in a 4% increase in static copies removed by this
pass when compiling the llvm test-suite. It also fixes regressions
caused by doing post-RA copy propagation (a separate change to be put up
for review shortly).

Event Timeline

gberry created this revision.Feb 17 2017, 12:37 PM
qcolombet added inline comments.Feb 17 2017, 3:50 PM
test/CodeGen/AArch64/machine-copy-remove.mir
274

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.

MatzeB accepted this revision.Feb 17 2017, 6:26 PM

LGTM with nitpicks. Please wait for qcolombet though.

lib/Target/AArch64/AArch64RedundantCopyElimination.cpp
80

Don't repeat the function name in the doxygen comments.

84–85

could add a continue; here

121

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?

127–128

could this be

if (!guaranteesZeroRegInBlock(...))
  continue;

to reduce indentation?

130–157

Should this have an additional check for situations in which all target regs are clobbered (similar to the one a few lines earlier?)

195–196

Drive-by-comment (even though the patch doesn't touch it), this can be done simpler as:
dbgs() << "Remove redundant Copy : " << *MI;

test/CodeGen/AArch64/machine-copy-remove.mir
4–13

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)

This revision is now accepted and ready to land.Feb 17 2017, 6:26 PM
gberry marked 7 inline comments as done.Feb 21 2017, 2:16 PM
gberry added inline comments.
lib/Target/AArch64/AArch64RedundantCopyElimination.cpp
121

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.

130–157

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.

test/CodeGen/AArch64/machine-copy-remove.mir
4–13

Good to know.

274

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.

gberry updated this revision to Diff 89279.Feb 21 2017, 2:16 PM
gberry marked 3 inline comments as done.

Update to address comments from Matthias and Quentin.

This revision was automatically updated to reflect the committed changes.