This is an archive of the discontinued LLVM Phabricator instance.

[PPC64] Add 64-bit Vector Integer Arithmetic Instructions
ClosedPublic

Authored by kbarton on Mar 3 2015, 1:18 PM.

Details

Summary

Clang-related changes for http://reviews.llvm.org/D7959

Add builtins for the following 64-bit vector integer arithmetic instructions added in POWER8:

vmulesw
vmulosw
vmuleuw
vmulouw
vmuluwm
vmaxsd
vmaxud
vminsd
vminud
vcmpequd
vcmpequd.
vcmpgtsd
vcmpgtsd.
vcmpgtud
vcmpgtud.
vrld
vsld
vsrd
vsrad

All builtins are added in altivec.h, and guarded with the POWER8_VECTOR macro.

Diff Detail

Event Timeline

kbarton updated this revision to Diff 21135.Mar 3 2015, 1:18 PM
kbarton retitled this revision from to [PPC64] Add 64-bit Vector Integer Arithmetic Instructions.
kbarton updated this object.
kbarton edited the test plan for this revision. (Show Details)
kbarton added reviewers: hfinkel, wschmidt, seurer, nemanjai.
kbarton added a subscriber: Unknown Object (MLST).
nemanjai added inline comments.Mar 4 2015, 4:05 AM
lib/Headers/altivec.h
1533

I am just curious why we are adding these. The ABI document specifies the [byte, halfword, word] versions of these as "deprecated and provided for compatibility only". Furthermore, it does not list the doubleword versions of these.

hfinkel added inline comments.Mar 4 2015, 7:43 AM
lib/Headers/altivec.h
1533

Generally speaking, a deprecated facility in a standard still needs to be provided by an implementation. However, if these are deprecated, we should add attribute((deprecated)) to these.

nemanjai added inline comments.Mar 4 2015, 7:49 AM
include/clang/Basic/BuiltinsPPC.def
220

Similar comment about the return type.

lib/Headers/altivec.h
2913

The ABI does not list any overloads for vmaxsd and vmaxud builtins other than the ones that take two vector [un]signed long long. Furthermore, gcc provides only the single overload as listed in the ABI. Are these for compatibility with something else?

3462

Similar comment to the above, both the ABI and gcc provide a single overload for each signed/unsigned version of this (no vector bool long long parameter overloads).

3640

Same comment about overloads, ABI/gcc have only one.

4042

Since the forms that are not type-generic are deprecated in the ABI, do we really want to add new ones? I suppose that for consistency, this is valuable.

4178

Similar comment about deprecated forms that are not type-generic.

6938

I am probably missing something, but it seems unclear to me how this will generate the expected instruction. If this code generates an "shr" in the IR and the back end generates a "vsrd" instruction when it sees the corresponding builtin, what tells the back end to emit the instruction when it sees a shift in the IR?

11318

Why the change in the return type for these? All the other ones return int, this one involves a promotion from in to long long. And the return type in the ABI is int.

11325

Once again, more overloads than the ABI lists. I assume we want these as well.

kbarton added inline comments.Mar 4 2015, 7:50 AM
lib/Headers/altivec.h
1533

I didn't realize these were deprecated in the ABI document. The corresponding byte, halfword, and word versions have not been marked as deprecated, so I added these for consistency.

I'll add the deprecated attribute to these now, and check the ABI document to see if others should be marked as deprecated also.

kbarton added inline comments.Mar 4 2015, 8:24 AM
lib/Headers/altivec.h
6938

You're right, this was an oversight on my part. I'll fix this, and make the necessary changes on the LLVM side.

11318

I'll fix this as well.

nemanjai added inline comments.Mar 4 2015, 8:35 AM
lib/Headers/altivec.h
1533

I think I was a bit unclear. The existing ones are deprecated, the new one is not listed in the ABI.

11513

Same comment about the return type.

11708

Return type again.

11912

Return type. I'll stop these comments, but basically the return type for all of the "compare all/any elements" builtins.

kbarton updated this revision to Diff 21499.Mar 9 2015, 11:01 AM

Addressed from previous review, plus cross-reference builtins with the new ABI document.

Notable changes:

  1. Fixed return types for the predicate compare functions to always use int instead of long
  2. Enabled vector bool long long when power8-vector is enabled (in addition to VSX)
  3. Removed builtins that are not included in the new POWER ABI document
  4. Removed the intrinsic for the vec_muluwm instruction as it can be generated using LLVM IR (multiplication of two v4i32 types)

The new interfaces added are slightly different from the existing interfaces in that they explicitly use the signed long long type (as opposed to implying it). This is done to be consistent with the new ABI document.

hfinkel accepted this revision.Mar 9 2015, 2:54 PM
hfinkel edited edge metadata.

Aside from some formatting issues, LGTM.

lib/Headers/altivec.h
3989

Remove blank line.

4125

Remove blank line.

11521

Line too long.

11548

Line too long.

11716

Line too long.

11743

Line too long.

11927

Line too long.

11948

Line too long.

12123

Line too long.

12144

Line too long.

12328

Line too long.

12582

Line too long.

12782

Line too long.

12803

Line too long.

12982

Line too long.

13183

Line too long.

13384

Line too long.

13608

Line too long.

lib/Sema/DeclSpec.cpp
992 ↗(On Diff #21499)

Line too long. (it was before too it seems, but you can fix it here).

test/CodeGen/builtins-ppc-p8vector.c
7

Only need one blank line here.

This revision is now accepted and ready to land.Mar 9 2015, 2:54 PM
nemanjai added inline comments.Mar 9 2015, 3:01 PM
include/clang/Basic/BuiltinsPPC.def
96

Is this the underlying builtin for the removed vec_muluwm? Should this be removed as well if so? The diff does not show any calls for this (i.e. in altivec.h or the test case).

include/clang/Basic/DiagnosticParseKinds.td
359 ↗(On Diff #21499)

Can we fix this message since the word "the" is out of place?
"available on POWER7 or later"
"available on POWER8 or later"

kbarton closed this revision.Mar 11 2015, 9:00 AM

Committed revision 231931.