This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Remove self-copies in pre-emit peephole
ClosedPublic

Authored by nemanjai on Sep 24 2018, 12:29 PM.

Details

Summary

There are occasionally instances where AADB rewrites registers in such a way that a reg-reg copy becomes a self-copy. Such an instruction is obviously redundant and can be removed. This patch does precisely that.

Note that this will not remove various nop's that we insert (which are themselves just self-copies). The reason those are left alone is that all of them have their own opcodes (that just encode to a self-copy).

What prompted this patch is the fact that these self-copies sometimes end up using registers that make the instruction a priority-setting nop, thereby having a significant effect on performance.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Sep 24 2018, 12:29 PM
hfinkel accepted this revision.Sep 25 2018, 7:13 AM

LGTM

lib/Target/PowerPC/PPCPreEmitPeephole.cpp
69

Please add a comment here stating that these can come out of the AADB.

This revision is now accepted and ready to land.Sep 25 2018, 7:13 AM
jsji added a comment.Sep 25 2018, 7:42 AM

It would be great if we can avoid generating self-copies in AADB and others (no sure whether it is easy or possible there). But yes, it is always great to put this sentinel guard here. Thanks!

lib/Target/PowerPC/PPCPreEmitPeephole.cpp
72

How about PPC::MCRF, PPC::CROR,PPC::EVOR? QPX copies if we still support QPX?

80

Maybe we should continue (skip the following check) once we identified that this is to be erased?

If the reviewers agree with my proposals for addressing the comments, I'll address them on the commit.

lib/Target/PowerPC/PPCPreEmitPeephole.cpp
69

Will add it on the commit. Thanks.

72

I didn't add the SPE and QPX ones because I can't test them. Of course, this should be a safe thing to do no matter what, but I am reluctant to add code I can't functionally test.
@hfinkel Do you think I should go ahead and add the following:

PPC::MCRF
PPC::QVFMR
PPC::QVFMRs
PPC::QVFMRb
PPC::CROR
PPC::EVOR
80

That's a good point. I'll update the patch to add a continue after either of the updates to InstrsToErase since we don't need to try to convert it to an R+I instruction if we know we're deleting it.

jsji accepted this revision.Sep 25 2018, 9:13 AM
hfinkel added inline comments.Sep 25 2018, 11:13 AM
lib/Target/PowerPC/PPCPreEmitPeephole.cpp
72

Seems safe enough. It has the benefit of not being an "arbitrary" subset. Downside is that it's harder to test. I'm fine with adding them in this case.

This revision was automatically updated to reflect the committed changes.