Page MenuHomePhabricator

[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
219

Similar comment about the return type.

lib/Headers/altivec.h
2872

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?

3380

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).

3534

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

3896

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.

4009

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

6704

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?

11050

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.

11057

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
6704

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

11050

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.

11245

Same comment about the return type.

11439

Return type again.

11641

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
3842

Remove blank line.

3955

Remove blank line.

11253

Line too long.

11280

Line too long.

11447

Line too long.

11474

Line too long.

11656

Line too long.

11677

Line too long.

11851

Line too long.

11872

Line too long.

12055

Line too long.

12309

Line too long.

12509

Line too long.

12530

Line too long.

12708

Line too long.

12909

Line too long.

13109

Line too long.

13332

Line too long.

lib/Sema/DeclSpec.cpp
992

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

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

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

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.