This is an archive of the discontinued LLVM Phabricator instance.

Add VSX Scalar loads and stores to the PPC back end
ClosedPublic

Authored by nemanjai on May 1 2015, 11:48 AM.

Details

Summary

This patch does two things:

  1. Adds the new VSX scalar loads and stores introduced in ISA 2.07
  2. Defines a new register class (VSSRC) similar to VSFRC that allows single precision (f32) values to be operated on in VSX registers.

In ISA 2.07, there is more complete support for single precision operations on the full set of VSX registers, so the new register class is needed.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 24823.May 1 2015, 11:48 AM
nemanjai retitled this revision from to Add VSX Scalar loads and stores to the PPC back end.
nemanjai updated this object.
nemanjai edited the test plan for this revision. (Show Details)
nemanjai added reviewers: wschmidt, hfinkel, kbarton, seurer.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: Unknown Object (MLST).
nemanjai added inline comments.May 1 2015, 11:52 AM
lib/Target/PowerPC/PPCInstrVSX.td
937

There is no semantic difference, but for consistency, this will be updated to replace xaddr with xoaddr.

1033

This was removed in response to a comment to the commit for the "direct moves" patch. The requirement is redundant since the DAG nodes only come from the custom lowering code and that code is guarded for direct moves.

wschmidt edited edge metadata.May 4 2015, 7:21 AM

In general this looks very good. Just a few inline comments from me. I haven't added a register class myself, so you may want a thumbs-up from someone with that experience, but I can't think offhand of anything you might have missed.

lib/Target/PowerPC/PPCInstrVSX.td
1033

This is a harmless redundancy and to me it seems worthwhile to leave in place. Code gets re-designed, and it may not be a redundant requirement in the future.

lib/Target/PowerPC/PPCRegisterInfo.cpp
305

&& Subtarget.hasVSX() ?

test/CodeGen/PowerPC/ppc64le-smallarg.ll
47

You don't have a run line specifying -mcpu, so this is potentially fragile.

kbarton added inline comments.May 4 2015, 1:09 PM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2714

You can drop the hasVSX() from this check - it is a prerequisite for hasP8Vector (see FeatureP8Vector definition in PPC.td)

lib/Target/PowerPC/PPCISelLowering.cpp
2673

Same here - hasVSX() can be dropped from the check

3091

Here as well - no hasVSX() needed.

lib/Target/PowerPC/PPCInstrVSX.td
100

One general comment, more organizational then anything else. In PPCInstrAltivec.td, we chose to put all the P8 instructions together, at the end of the file, instead of intermixing them with existing instructions. Do we want to do the same thing in this file as well, if for no other reason then to be consistent?

lib/Target/PowerPC/PPCRegisterInfo.cpp
305

nope, shouldn't be needed here :)

nemanjai added inline comments.May 5 2015, 5:08 AM
lib/Target/PowerPC/PPCISelDAGToDAG.cpp
2714

I was under the impression that specifying -mattr=-vsx would not automatically turn off power8-vector, but you are right it does. I'll remove the redundant checks. Unless anyone has any objections to this.

lib/Target/PowerPC/PPCInstrVSX.td
100

It is all the same to me. I just put them here because the loads/stores are here. I can just as easily move them. Are we all in agreement that this is what we want?

1033

Noted. I will put it back in.

test/CodeGen/PowerPC/ppc64le-smallarg.ll
47

I can certainly put it in, but I avoided doing that for 2 reasons:

  1. There is metadata in the test case identifying the target triple (which LLC respects) so the features are getting picked up due to the powerpc64le arch. This makes it work on other platforms too (tried on X86).
  2. Since I didn't write the test case, I am not sure if the intent indeed was to not specify the -mcpu for some reason but rather provide the triple in the IR metadata.

Please confirm that you would like me to remove the

target triple = "powerpc64le-unknown-linux-gnu"

and add -mcpu=pwr8. I can then put back the original checks for other CPU's.

wschmidt added inline comments.May 5 2015, 5:35 AM
lib/Target/PowerPC/PPCInstrVSX.td
100

I agree with Kit; let's try to keep the P8Vector stuff together.

test/CodeGen/PowerPC/ppc64le-smallarg.ll
47

No, after reading your comments I'm okay with leaving it as is. Relying on the arch for POWER8 is ok, but bear in mind that instructions for any follow-on ISA in the future won't be able to do this.

hfinkel edited edge metadata.May 5 2015, 6:15 PM

Modulo comments already made by others, this LGTM. I assume you'll be adding the other f32 VSX instructions in the near future.

nemanjai updated this revision to Diff 25171.May 7 2015, 6:46 AM
nemanjai edited edge metadata.

Addressed the review comments.

  • Removed redundant checks for VSX
  • Moved all the P8Vector loads and stores down to the bottom of PPCInstrVSX.td
  • Put the check for Direct Moves back in (for clarity and possible future needs)

VSX scalar arithmetic instructions to follow.

wschmidt accepted this revision.May 7 2015, 7:31 AM
wschmidt edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.May 7 2015, 7:31 AM
nemanjai closed this revision.May 7 2015, 11:27 AM

Committed revision 236755.