This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix for excessive ACC copies due to PHI nodes
ClosedPublic

Authored by bsaleil on Nov 12 2020, 2:03 PM.

Details

Summary

When using accumulators in loops, they are passed around in PHI nodes of unprimed accumulators, causing the generation of additional
prime/unprime instructions. This patch detects these cases and changes these PHI nodes to primed accumulator PHI nodes.
We also add IR and MIR test cases for several PHI node cases.

Diff Detail

Event Timeline

bsaleil created this revision.Nov 12 2020, 2:03 PM
bsaleil requested review of this revision.Nov 12 2020, 2:03 PM
amyk added a subscriber: amyk.Nov 12 2020, 3:43 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
289

Do you mean we check if they can be converted?

llvm/test/CodeGen/PowerPC/peephole-phi-acc.mir
188

Are these necessary to have here/can they be omitted?

bsaleil updated this revision to Diff 304998.Nov 12 2020, 5:04 PM

Remove metadata from test files and fix comment

bsaleil updated this revision to Diff 305002.Nov 12 2020, 5:18 PM
bsaleil marked 2 inline comments as done.

Remove test case metadata and attributes

nemanjai accepted this revision.Dec 1 2020, 7:39 PM

My comments are all cosmetic, feel free to address them on the commit. LGTM otherwise.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
318

This comment appears to be out of date. It talks about returning false but the return type is void. Please revise the comment.

363

Please add a comment here describing why we are creating a new primed accumulator virtual register as it is not trivially obvious.
Perhaps something like

If the PHI node we are changing is the root node, the register it defines will
be the destination register of the original copy (of the PHI def). For all other
PHI's in the list, we need to create another primed accumulator virtual
register as the PHI will no longer define the unprimed accumulator.
439

s/directions/direction

440

s/satisfying (i) (ii) and (iii)/satisfying (i) and (ii)
no need for a recursive description :)

This revision is now accepted and ready to land.Dec 1 2020, 7:39 PM
bsaleil updated this revision to Diff 309064.Dec 2 2020, 2:46 PM

Fix comments

bsaleil updated this revision to Diff 309070.Dec 2 2020, 3:06 PM
bsaleil marked 4 inline comments as done.

Update patch so it applies to ToT

saghir accepted this revision.Dec 3 2020, 6:38 AM

LGTM.

This revision was automatically updated to reflect the committed changes.