This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Power10] Exploit the xxspltiw and xxspltidp instructions.
ClosedPublic

Authored by lei on Jun 30 2020, 1:20 PM.

Details

Summary

This patch exploits the VSX Vector Splat Immediate Word and VSX Vector Splat Immediate Double Precision Instructions:

xxspltiw XT,IMM32
xxspltidp XT,IMM32

Depends on D82896.

Diff Detail

Unit TestsFailed

Event Timeline

amyk created this revision.Jun 30 2020, 1:20 PM
lei added inline comments.Jun 30 2020, 3:14 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1477

Any reason why we are not following the format used for the other case statements?
If it fit within 1 line, it would be good to match the formate of this switch stmt.

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

nit: indentation is off.

llvm/test/CodeGen/PowerPC/power10-immediate-moves-and-splats.ll
4 ↗(On Diff #274602)

Do we need BE run line here?

amyk updated this revision to Diff 274644.Jun 30 2020, 4:27 PM

Updated the patch:

  • to include BE RUN line in the test
  • adjust indentation
  • add functions to the llvm namespace.
amyk marked 2 inline comments as done.Jun 30 2020, 4:30 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
708

I'm not entirely sure why the indentation appears like that, as the indentation is correct in the patch... (under v2f64).

llvm/test/CodeGen/PowerPC/power10-immediate-moves-and-splats.ll
4 ↗(On Diff #274602)

I've added the BE line for now as different code is generated for tests similar to testDoubleToDoubleFail. If there is a preference to remove the BE line, I can remove it.

amyk added a reviewer: anil9.Jun 30 2020, 4:40 PM
amyk updated this revision to Diff 274657.Jun 30 2020, 5:03 PM

Updated the patch:

  • to include proper test case to account for PC Rel.
  • add BE run line to the test case.
amyk marked an inline comment as done.Jun 30 2020, 5:04 PM

FYI @lei regarding the test case.

llvm/test/CodeGen/PowerPC/power10-immediate-moves-and-splats.ll
4 ↗(On Diff #274602)

I realized that the correct test case has checks to account for PC Rel. I have updated the revision and kept the BE run lines for now.

amyk updated this revision to Diff 274668.Jun 30 2020, 7:35 PM

Make changes to adhere to clang-format.

amyk marked an inline comment as done.Jun 30 2020, 7:36 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1477

This line is actually due to clang-format.

lei added inline comments.Jun 30 2020, 8:07 PM
llvm/test/CodeGen/PowerPC/power10-immediate-moves-and-splats.ll
11 ↗(On Diff #274668)

Was thinking maybe we can break this test file into 2 different ones:

  1. p10-splatsImm.ll
    • contain tests that produces the same code for both LE/BE (not dependent of PCRel).
    • both run lines can use default for check prefix.
  2. p10-splatsImm-pcrel.ll
    • contain tests that produce code dependent on whether PCRel is supported or not
    • Use default for PCRel enabled (LE tests) and CHECK-TOC for when it's not (ie BE and when PCRel is disabled).
lei added inline comments.Jun 30 2020, 8:19 PM
llvm/test/CodeGen/PowerPC/power10-immediate-moves-and-splats.ll
11 ↗(On Diff #274668)

I meant p10-splatImm.ll and p10-splatImm-no-pcrel.ll

lei commandeered this revision.Jul 1 2020, 10:35 AM
lei edited reviewers, added: amyk; removed: lei.

Taking over ownership to address review comments.

lei updated this revision to Diff 274867.Jul 1 2020, 10:37 AM
  • clean up condition code in isFPImmLegal()
  • split test file into pcrel and non-pcrel related, clean up checks
  • add test for BE elfv2
anil9 added inline comments.Jul 1 2020, 10:49 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9338–9351

This fixme comment is no longer relevant as the fix has already been applied.

NeHuang added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9342

Is it better to combine these two conditions? Seems only the first argument of getCanonicalConstSplat depends on SplatSize.

if (Subtarget.hasPrefixInstrs() && (SplatSize == 2 || SplatSize == 4))
  return getCanonicalConstSplat ((SplatSize == 2) ? (SplatBits |= SplatBits << 16) : SplatBits, 4, Op.getValueType(), DAG, dl)
16324

Undo this change.

anil9 added inline comments.Jul 1 2020, 12:08 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9342

The current code is more readable than the suggested one, and was decided on after a discussion. I prefer the way it is now.

lei updated this revision to Diff 274890.Jul 1 2020, 12:22 PM
lei marked 4 inline comments as done.

Remove unnecessary FIXME message, accidental formating, and simplify if-condition as suggested by Victor.

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

I can do that, don't feel strongly about it either way.

LGTM other than the minor nit.

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

+1

llvm/test/CodeGen/PowerPC/p10-splatImm-pcrel.ll
1 ↗(On Diff #274890)

Seems that this test case should have something in its name to indicate that none of the splats can use the new splat instructions.

lei updated this revision to Diff 274893.Jul 1 2020, 12:40 PM

revert changes to the if condition

anil9 accepted this revision.Jul 1 2020, 12:42 PM

LGTM

This revision is now accepted and ready to land.Jul 1 2020, 12:42 PM
nemanjai accepted this revision.Jul 1 2020, 12:44 PM

LGTM.

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

Maybe add a comment to state that this includes positive zero.

lei updated this revision to Diff 274900.Jul 1 2020, 1:14 PM
lei marked an inline comment as done.

rename test file and add additional ccomments to code.

lei marked 2 inline comments as done.Jul 1 2020, 1:17 PM
lei added inline comments.
llvm/test/CodeGen/PowerPC/p10-splatImm-pcrel.ll
1 ↗(On Diff #274890)

Will rename to test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll on commit.

This revision was automatically updated to reflect the committed changes.