This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix the xxperm swap requirements
ClosedPublic

Authored by maryammo on Mar 22 2023, 7:32 AM.

Details

Summary

This patch is to fix the xxperm vector operand swap condition so that the
single-use operand is in V2 to prevent copying, it also fixes the subtarget
condition to exploit the xpperm.

Diff Detail

Event Timeline

maryammo created this revision.Mar 22 2023, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 7:32 AM
maryammo requested review of this revision.Mar 22 2023, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 7:32 AM

I realize that a lot of test cases have changed but it would be good to add the test case that is specific to this opportunity where the copy is removed.

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

nit:
Too many // .

Also, this comment needs to do a better job explaining why we want to avoid having the input with multiple uses as the second input to XXPERM. ie. That we have one input that is also an output for XXPERM and so that input must be clobbered.

llvm/test/CodeGen/PowerPC/pre-inc-disable.ll
140

Here we are replacing lfiwzx with lxsiwzx.
This could technically be a (very minor...) performance hit as we are now using an operation that is 5 cycles instead of 4.
I guess the upside is that we have a wider range of result registers.

182

Here it is the same idea as above. We are replacing a 4 cycle with a 5 cycle.

maryammo updated this revision to Diff 509107.Mar 28 2023, 12:58 PM

Address review comments

stefanp accepted this revision.Apr 3 2023, 1:35 PM

I have only one nit and that can be fixed on commit.
Otherwise LGTM.

llvm/test/CodeGen/PowerPC/xxperm-swap.ll
96 ↗(On Diff #509107)

nit:
See if you can cut this list down. There are a lot of tests that have only required nounwind.
After you cut down the list make sure that the test still passes.

This revision is now accepted and ready to land.Apr 3 2023, 1:35 PM
nemanjai added inline comments.Apr 3 2023, 2:10 PM
llvm/test/CodeGen/PowerPC/xxperm-swap.ll
1 ↗(On Diff #509107)

Nit: it appears that the checks were produced bu update_llc_test_checks.py yet the comment saying so at the top of the file is not present (removed?). If you're using it to produce the checks, please leave it in place as it signals to future maintainers that it is safe to use it to update the test if needed.

96 ↗(On Diff #509107)

+1

maryammo updated this revision to Diff 511205.Apr 5 2023, 2:10 PM

Cut down the fn attributes and use update_llc_test_checks.py to generate checks

This revision was automatically updated to reflect the committed changes.
This comment was removed by maryammo.
llvm/test/CodeGen/PowerPC/pre-inc-disable.ll
140

Thanks, I am just not sure about the solution if there is any?

maryammo added a comment.EditedApr 5 2023, 6:42 PM

The patch is causing bot failures which did not show up at the time of posting this:
https://lab.llvm.org/buildbot/#/builders/16/builds/46086/steps/6/logs/FAIL__LLVM__pr61315_ll
TEST 'LLVM :: CodeGen/PowerPC/pr61315.ll' FAILED
I will post another patch to update that test accordingly.