This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Update Refactored Load/Store Implementation, XForm VSX Patterns, and Tests
ClosedPublic

Authored by amyk on Jan 20 2021, 10:56 PM.

Details

Summary

This patch includes the following updates to the load/store refactoring effort introduced in D93370:

  • Update various VSX patterns that use to "force" an XForm, to instead just XForm. This allows the ability for the patterns to compute the most optimal addressing mode (and to produce a DForm instruction when possible)
  • Update pattern and test case for the LXVD2X/STXVD2X intrinsics
  • Update LIT test cases that use to use the XForm instruction to use the DForm instruction

Depends on D93370 and D105661

Diff Detail

Event Timeline

amyk created this revision.Jan 20 2021, 10:56 PM
amyk requested review of this revision.Jan 20 2021, 10:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 10:56 PM
amyk updated this revision to Diff 319727.Jan 27 2021, 6:11 PM

Rebase patch and also add additional handling for frame index (if the frame index is not aligned, match to an XForm instruction).

amyk updated this revision to Diff 322977.Feb 11 2021, 6:03 AM
  • Rebase the patch with the latest trunk.
  • Updated alignment checks for the frame index.
amyk updated this revision to Diff 323859.Feb 15 2021, 6:04 PM

Update patch based on dependent patch changes.

NeHuang added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16647 ↗(On Diff #323859)

nit: remove duplicate only

16657 ↗(On Diff #323859)

nit: double check if the frame index is aligned

amyk updated this revision to Diff 330638.EditedMar 15 2021, 6:42 AM

Update patch based on dependent patch (D93370), update newly added test cases, address review on comment updates.

nemanjai requested changes to this revision.Mar 22 2021, 5:04 AM

It is really nice to see us getting rid of all these unnecessary X-Forms in so many test cases. But I think the patch needs a bit more tweaking.

Also, it seems that if we commit the first patch in this series before we commit this one, there is a possibility of emitting an impossible DSForm/DQForm instruction (since the alignment check for the FI is only added here). Can you explain why that is not the case? I think for DQ-Form it is because we only produce DQ-Forms starting with this patch, but I think it is possibly a problem with DS-Form.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
16812 ↗(On Diff #330638)

Similarly to my comment on the other lambda in the first patch in the series, please turn this lambda into a static function. Also, the name is not appropriate. If a function is named as a query, it should be a query - calling IsFrameIndexAligned() seems like the caller is asking a question and would expect an answer.
Perhaps setAlignFlagsForFI()?

16814 ↗(On Diff #330638)

How about if the address is computed using OR? I am fairly certain that the original code handles this case.

16819 ↗(On Diff #330638)

A comment explaining something like this seems appropriate:

If this is (add $FI, $S16Imm), the alignment flags are already set based on the immediate. We just need to clear the alignment flags if the FI alignment is weaker.
16824 ↗(On Diff #330638)
// If the address is a plain FrameIndex, set alignment flags based on FI alignment.
16939 ↗(On Diff #330638)

I don't really follow why we need this at all. This function is meant to set the flags. This preemptive look at which addressing mode we will produce for the flags we computed so far seems out of place.
Presumably simply calling IsFrameIndexAligned(N) ensure that PPC::MOF_RPlusSImm16Mult4/FlagSet & PPC::MOF_RPlusSImm16Mult16 are not set on unaligned frame indices. And presumably, if those flags are not set, we'd never try to match AM_DSForm/AM_DQForm and would simply fall back to AM_XForm. It might help if you provide a test case that requires this code.

This revision now requires changes to proceed.Mar 22 2021, 5:04 AM
amyk updated this revision to Diff 333292.Mar 25 2021, 6:59 AM
amyk marked 4 inline comments as done.
amyk edited the summary of this revision. (Show Details)
  • Moved setting FrameIndex alignment flags implementation to D93370 (with added comments from Nemanja's review)
amyk updated this revision to Diff 338418.Apr 18 2021, 7:48 PM
  • Rebase patch in order to update test cases
nemanjai accepted this revision.May 7 2021, 6:26 AM

LGTM. This turned out very nice.

This revision is now accepted and ready to land.May 7 2021, 6:26 AM

Is there anything additional I should be reviewing here? Were there any changes required when rebasing that you need me to look at specifically?

amyk added a comment.Jul 16 2021, 7:30 AM

Is there anything additional I should be reviewing here? Were there any changes required when rebasing that you need me to look at specifically?

Sorry Nemanja, I just seen this. This patch didn't change, I just needed to commit it (which I just did). Thanks.