This is an archive of the discontinued LLVM Phabricator instance.

[PPC64] Add v1i128 Vector Type
ClosedPublic

Authored by kbarton on Mar 23 2015, 1:57 PM.

Details

Summary

Add support for quadword vector instructions added in POWER8.
In order to implement these instructions it is necessary to introduce the v1i128 vector type in LLVM. The PowerPC ABI specifies parameters of type v1i128 are to be passed in a single vector register, while parameters of type i128 are passed in pairs of GPRs. Thus it is necessary to be able to differentiate between v1i128 and i128 in LLVM.

There will be a subsequent patch following soon that uses the v1i128 type for the add and subtract quadword instructions introduced in POWER8.

Diff Detail

Event Timeline

kbarton updated this revision to Diff 22513.Mar 23 2015, 1:57 PM
kbarton retitled this revision from to [PPC64] Add v1i128 Vector Type.
kbarton updated this object.
kbarton edited the test plan for this revision. (Show Details)
wschmidt edited edge metadata.Mar 23 2015, 3:10 PM

Just a couple of nits. I haven't tried to add a new type, so I don't know where the pitfalls are -- will let others speak to that.

As Uli mentioned last week, you'll also have to deal with the same type issues in the front end in order to handle the ABI issues -- so more type hierarchies to deal with there...

lib/Target/PowerPC/PPCISelLowering.cpp
626 ↗(On Diff #22513)

SUB?

test/CodeGen/PowerPC/vec_add_sub_quadword.ll
1 ↗(On Diff #22513)

128?

kbarton added inline comments.Mar 24 2015, 8:06 AM
lib/Target/PowerPC/PPCISelLowering.cpp
626 ↗(On Diff #22513)

This is a good point!
SUB is working because there is a general statement above that makes ADD/SUB legal for all vector types.
However, this reminds me that we are probably marking a lot of instructions legal for v1i128 that need to be marked as expand.

kbarton updated this revision to Diff 23420.Apr 8 2015, 8:40 AM
kbarton edited edge metadata.

New patch with some changes - including the addition of the get methods in Constants.h (which use the __uint128_t type).
I've also removed the PPC-specific changes from this patch, as we decided it makes more sense to submit them in a separate (follow-on) patch.

seurer edited edge metadata.Apr 8 2015, 9:18 AM

Very "nitty" style nit (for which I always get flagged): put periods at the end of comments.

kbarton updated this object.Apr 10 2015, 8:26 AM
kbarton edited the test plan for this revision. (Show Details)
kbarton edited edge metadata.
kbarton added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Apr 11 2015, 7:20 PM
include/llvm/CodeGen/MachineValueType.h
29

Don't add blank line.

include/llvm/IR/Constants.h
676

We have don't have a standard __uint128_t type, so you can't use it.

Also, you don't define these anywhere. Do you need them?

kbarton added inline comments.Apr 14 2015, 7:53 AM
include/llvm/IR/Constants.h
676

This is one of the changes that I had questions about.
I found this while I was looking through svn history of other patches that had added types to LLVM. I haven't tried to add any tests that contain 128bit constants, but I would expect these methods to be used in that scenario (that's purely speculation on my part).

I'm OK with not modifying this, since it is not needed right now to compile/run. I added it originally for completeness.

kbarton updated this revision to Diff 23735.Apr 14 2015, 8:49 AM

Removed the get(LLVMContext &Context, ArrayRef<__uint128_t> Elts) constructors in Constants.h.

hfinkel accepted this revision.Apr 16 2015, 10:16 AM
hfinkel edited edge metadata.

LGTM. But please break this into two pieces when you commit: the IR/Core pieces for i128 and the CodeGen bits for v1i128 with distinct commit messages explaining why each is needed.

This revision is now accepted and ready to land.Apr 16 2015, 10:16 AM
kbarton closed this revision.Apr 17 2015, 9:14 AM

i128 patch: Committed revision 235196.
v1i128 patch: Committed revision 235198.