This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Remove redundant TOC saves
ClosedPublic

Authored by syzaara on Nov 7 2017, 8:29 AM.

Details

Summary

This patch adds a peep hole optimization to remove any redundant toc save instructions added as part of the call sequence for indirect calls. It removes any toc saves within a function that are dominated by another toc save.

Diff Detail

Event Timeline

syzaara created this revision.Nov 7 2017, 8:29 AM
nemanjai added inline comments.Nov 7 2017, 11:22 AM
lib/Target/PowerPC/PPCMIPeephole.cpp
81

Please don't copy the list by passing it by-value.

215

I know that this won't really change for the ABI you're doing this on, but the TOC frame offset is available in PPCFrameLowering::getTOCSaveOffset(). In fact, I would extract the logic into a function like PPCInstrInfo::isTOCSaveMI(MachineInstr &) so that we contain the logic in one place if we want to handle other ABI's, etc.

217

These won't change within the enclosing loop. So why not check this first and don't bother with any of the other checks if this doesn't pass (i.e. this should be the first early exit from this case).

914

To be honest with you, I kind of cringe when I see code that does a quadratic iteration over a container and erases elements from the container, invalidating the iterators.
I don't know if it is a problem if the iterator you're using to erase is before the iterator in the outer loop. How about before?

I think I'd opt for a solution that does all the erasing in a single pass over the data structure. Whether the easiest way to achieve that is to maintain two lists (ToErase and ToKeep) or converting this to a map<MachineInstr *, bool> where the bool would be set if the instruction is proven safe to erase, or something entirely different...

syzaara updated this revision to Diff 122503.Nov 10 2017, 12:56 PM
nemanjai edited edge metadata.Nov 14 2017, 4:18 PM

Also, for the test cases that have to keep multiple TOC saves, can you just add a comment as to why it isn't safe to remove one (i.e. neither dominates the other, etc.).

lib/Target/PowerPC/PPCMIPeephole.cpp
192

If we already know we're not keeping the instruction at the iterator point, no reason to check.

if (It->second && MDT->dominates(MI, CurrInst))
  It->second = false;
198

It should be safe to break out of the loop here. Dominance is transitive. If CurrInst dominates MI, it will also dominate anything that MI dominates - and when CurrInst was inserted, it has updated all instructions it dominates.

915

I think this can be a range-based for loop now.

syzaara updated this revision to Diff 123228.Nov 16 2017, 11:53 AM

LGTM.
The remaining comments are essentially just minor nits.

lib/Target/PowerPC/PPCMIPeephole.cpp
187

Please add an assert that MI is the right type of instruction. Perhaps

assert(TII.isTOCSaveMI(MI) &&
       "Expecting a TOC save instruction here");
202

Formatting nit - no space after the left paren. It also seems more natural to use the array access operator here:
TOCSaves[MI] = Keep;

915

I think it would be useful to have a statistic that would count the number of functions that have multiple TOC saves that must be kept here. You would then increment that statistic if you hit more than one element here that has .second == true.

nemanjai accepted this revision.Nov 22 2017, 6:27 AM
This revision is now accepted and ready to land.Nov 22 2017, 6:27 AM
This revision was automatically updated to reflect the committed changes.