This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Exploit Prefixed Load/Stores using the refactored Load/Store Implementation
ClosedPublic

Authored by amyk on Feb 4 2021, 1:20 PM.

Details

Summary

This patch exploits the prefixed load and store instructions utilizing the refactored load/store
implementation introduced in D93370.

Prefixed load and store instructions are emitted whenever we are loading or storing a value with an
offset that fits into a 34-bit signed immediate.
Patterns for the prefixed load and stores are added in this patch, as well as the implementation that
detects when we are loading and storing a value with an offset that fits in 34-bits.

Depends on D95116.

Diff Detail

Event Timeline

amyk created this revision.Feb 4 2021, 1:20 PM
amyk requested review of this revision.Feb 4 2021, 1:20 PM
amyk edited the summary of this revision. (Show Details)Feb 5 2021, 10:02 AM
amyk updated this revision to Diff 322984.Feb 11 2021, 6:08 AM
  • Update LIT test cases with prefixed load/store checks.
  • Added a PrefixInstrs flag.
amyk edited the summary of this revision. (Show Details)Feb 11 2021, 6:11 AM
amyk updated this revision to Diff 323861.Feb 15 2021, 6:09 PM

Address clang-format comments.

amyk updated this revision to Diff 330641.Mar 15 2021, 6:44 AM
amyk added a reviewer: stefanp.

Update patch based on dependent patch (D95116) update, and update test cases with necessary changes.

Looking at the test case changes there seem to be a few places where we are not generating expected code.
I've added comments to some of those places.

llvm/test/CodeGen/PowerPC/prefixed-ld-st.ll
364 ↗(On Diff #330641)

Hmm... that's interesting.
This is probably not related to this patch but this code looks odd.
For starters pstxsd is a D-Form instruction but we are listing two registers and no immediate.

Also,

pli r4, 4294967297
add r3, r3, r4

can probably be written as a paddi.

paddi r3, r3, 4294967297, 0

I feel like we need a TODO for this.

llvm/test/CodeGen/PowerPC/scalar-double-ldst.ll
3134

This doesn't look right.
I assume we are trying to incorporate the pli into the lfsx but somehow the compiler gets confused and doesn't do that. Also, plfs is a D-Form and should have an immediate.

3259

We have expanded the pli into two instructions at this point which we shouldn't do.
Again, I think we are trying to merge pli and lfsx into plfs but that's not working as expected.

amyk updated this revision to Diff 339842.Apr 22 2021, 7:17 PM
  • Rebase patch
  • Address review comment made by Stefan involving plfs and pstxsd using two registers such as pstxsd v2, r3(r3), 0 (despite being a prefixed DForm instruction) instead of a register and an immediate. This was caused by the pattern for these instructions being written incorrectly. The patterns have been updated accordingly so the prefixed instructions use a register and an immediate.
amyk added a comment.May 7 2021, 7:45 AM

Gentle ping.

LGTM other than the missing assert. Also, please look into the interaction between prefixed instructions, pc-relative memops, paired memops and whether we do the right thing when 1/2 of those are turned on and others turned off (this can be done in a subsequent patch).

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
17394

I don't really understand what ensures that the intrinsic is one of ppc_vsx_lxvp/ppc_vsx_stxvp? What if it is something different entirely? It may be that we only call this function for those two now, but think about the future as well.
Even an assert would probably do here.

llvm/lib/Target/PowerPC/PPCISelLowering.h
712

I added a comment in the previous revision that MOF_SubtargetP10 should really be something like MOF_PrefixInstrs but perhaps I missed something. Is there any reason we need both? Are there conditions where we want a different non-prefixed addressing mode for P10 instructions?

nemanjai accepted this revision.May 10 2021, 4:41 AM

I thought I had selected "Accept" when adding the last comment.

This revision is now accepted and ready to land.May 10 2021, 4:41 AM
amyk updated this revision to Diff 364999.Aug 7 2021, 8:39 PM

Discussed offline with Nemanja to update the revision.

  • Add assert for the paired vector intrinsics
  • Add assert for 256-bit vectors

I've also changed PPCRegisterInfo.cpp to take prefixed instructions into account.

amyk added a comment.Aug 9 2021, 9:39 AM

@nemanjai I've updated the prefixed load/store patch once more, if you could take another look when you get the chance to see if I have addressed the comments that we discussed about offline, that would be great.

nemanjai accepted this revision.Sep 8 2021, 3:27 AM

LGTM.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2707

Nit: this line appears to be too long (and so does 2710).

This revision was landed with ongoing or failed builds.Sep 14 2021, 6:40 AM
This revision was automatically updated to reflect the committed changes.