This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Implement vec_xxsldwi builtin.
ClosedPublic

Authored by jtony on May 16 2017, 6:54 AM.

Details

Summary

Note this patch is based on the infrastructure in patch https://reviews.llvm.org/D33053 . The vec_xxsldwi builtin is also missing from altivec.h. This has been requested by developers working on libvpx for VP9 support for Google ( PR: https://bugs.llvm.org/show_bug.cgi?id=32653). This patch implements the vec_xxsldwi builtin by using the existing shuffleVector instruction just in the FE. The implementation here is similar to vec_xxpermdi in the patch D33053.

And we currently won't emit XXSLDWI in the assembly, subsequent BE optimization work (see patch D33225) needed to identify the correct shuffles so that we can emit XXSLDWI.

Diff Detail

Repository
rL LLVM

Event Timeline

jtony created this revision.May 16 2017, 6:54 AM
nemanjai requested changes to this revision.May 16 2017, 11:30 AM

The changes required for this patch aren't too different from those required for the other patch. Just basically a bit more testing and making the code a bit more readable.

lib/CodeGen/CGBuiltin.cpp
8517 ↗(On Diff #99135)

We don't want to clamp this. We should just use the low order two bits. A simple
int64_t Index = ArgCI->getSExtValue() & 0x3;
Should suffice. (although it seems like unsigned and getZExtValue() should be used, but it doesn't really make a difference here).

8529 ↗(On Diff #99135)

The indentation of the code in all the cases is off. It should be indented from the case label. In any case, I feel once again that a switch statement is a bit overkill and doesn't really provide additional insight into where the numbers came from.

For example, say you had expressions like this for the elements:

ElemIdx0 = (8 - Index) % 8;
ElemIdx1 = (9 - Index) % 8;
ElemIdx2 = (10 - Index) % 8;
ElemIdx3 = (11 - Index) % 8;

That is easy enough to explain in a comment such as:

// Little endian element N comes from element 8+N-Index of the 
// concatenated wide vector (of course, using modulo arithmetic on
// the total number of elements).
8556 ↗(On Diff #99135)

Of course, the expressions to replace the switch here are rather obvious...
ElemIdx<N> = Index + N

8593 ↗(On Diff #99135)

This will require the same type conversion as the other patch. I imagine it would crash on something like this:

vector double test(vector double a, vector double b) {
  return vec_xxsldwi(a, b, 1);
}

And of course, add that as a test case.

This revision now requires changes to proceed.May 16 2017, 11:30 AM
jtony edited the summary of this revision. (Show Details)May 17 2017, 5:59 PM
jtony updated this revision to Diff 99385.May 17 2017, 7:48 PM
jtony edited edge metadata.
jtony marked 3 inline comments as done.

Address comments from Nemanja.

jtony marked an inline comment as done.May 17 2017, 7:48 PM

LGTM other than the test case.

test/CodeGen/builtins-ppc-error.c
35 ↗(On Diff #99385)

Test for non-vector arguments.

inouehrs added inline comments.May 19 2017, 2:20 AM
lib/CodeGen/CGBuiltin.cpp
8479 ↗(On Diff #99385)

minor: uint64_t (or unsigned) for zext value?

8484 ↗(On Diff #99385)

minor: unsigned is more common than unsigned int

test/CodeGen/builtins-ppc-error.c
31 ↗(On Diff #99385)

Adding const is good here. (https://reviews.llvm.org/D33053#759232)

jtony updated this revision to Diff 99975.May 23 2017, 12:45 PM
jtony marked 4 inline comments as done.

Address minor comments from Nemanja and Hiroshi.

nemanjai accepted this revision.May 23 2017, 1:47 PM

Aside from the minor nit, this LGTM.

lib/CodeGen/CGBuiltin.cpp
8501 ↗(On Diff #99975)

Minor nit: no need for the + 0.

This revision is now accepted and ready to land.May 23 2017, 1:47 PM
echristo accepted this revision.May 23 2017, 5:34 PM

Sadly I think constant checking plus the shift makes it hard to do with just __builtin_shufflevector in altivec.h.

LGTM.

-eric

jtony marked an inline comment as done.May 24 2017, 8:32 AM
This revision was automatically updated to reflect the committed changes.