Page MenuHomePhabricator

[PowerPC] Simplify a Swap if it feeds a Splat
AbandonedPublic

Authored by stefanp on Nov 9 2017, 12:18 PM.

Details

Summary

Modify the peephole optimization for PowerPC so that it doesn't just simplify a shift feeding a splat but also a swap feeding a splat.

Also fixed a small bug in the original shift feeds splat code.

Diff Detail

Event Timeline

stefanp created this revision.Nov 9 2017, 12:18 PM
nemanjai accepted this revision.Nov 9 2017, 2:19 PM

This looks good, but of course, lets run this through all the testing before committing. Also, I think that for the test cases you added, you don't expect to see any swaps. You should be able to add -implicit-check-not to the RUN lines.
With these minor changes, LGTM.

lib/Target/PowerPC/PPCMIPeephole.cpp
403

Since you're adding ImmOpNo as well, please rename this to RegOpNo to make it clear. I know we generally don't rename things in the patch like this, but I think it's OK here because it is the introduction of the new one that requires this one to be renamed.

444

It's fine to just get rid of this and turn it into an assert. Just assert that the expected operand is an immediate.

467

Add a comment such as:

// Note that in both cases, the immediate represents the
// number of words the input vector is rotated left.
// For example: xxsldwi x, y, y, 2 == xxpermdi x, y, y, 2.
470
// If the two register operands differ, this isn't a
// shift/swap but a permutation of a pair of concatenated
// registers.
This revision is now accepted and ready to land.Nov 9 2017, 2:19 PM
stefanp updated this revision to Diff 132844.Feb 5 2018, 9:04 AM

Fixed the issue where we could have assigned a VSR register to an instruction that requires a VR register.

stefanp requested review of this revision.Feb 5 2018, 9:06 AM
nemanjai accepted this revision.Feb 5 2018, 11:48 AM

Thank you for fixing the issue with the register class - it has clearly existed in this code before your patch as well. My comments are minor nits.

The patch looks good to me, please run some tests with the verifier turned on and commit when you have a clean result.

lib/Target/PowerPC/PPCMIPeephole.cpp
490

I think it would be better use unreachable here since we really shouldn't be in this block.

test/CodeGen/PowerPC/ppc-xxsldwi-peephole.ll
15

I would much prefer that we specify what we're testing here a little more strictly.

  • The output register will be 2 since that's mandated by the ABI
  • The immediate should be specified
  • The blr should be a CHECK-NEXT
test/CodeGen/PowerPC/ppc64-peephole-swap.ll
5

Just a minor nit (especially since this is in a test case): we never indent anything by 1 space. If it is indented, it is indented by a multiple of 2 spaces.

This revision is now accepted and ready to land.Feb 5 2018, 11:48 AM
stefanp abandoned this revision.Feb 3 2021, 3:55 AM

This feature has already been covered by https://reviews.llvm.org/D77448
I am going to abandon this review.