Page MenuHomePhabricator

[PPC] Implement vector reverse elements builtins (vec_reve)
ClosedPublic

Authored by jtony on Oct 24 2016, 7:45 AM.

Details

Summary

This patch introduces the following builtins:

vector bool char vec_reve (vector bool char);
vector signed char vec_reve (vector signed char);
vector unsigned char vec_reve (vector unsigned char);
vector bool int vec_reve (vector bool int);
vector signed int vec_reve (vector signed int);
vector unsigned int vec_reve (vector unsigned int);
vector bool long long vec_reve (vector bool long long);
vector signed long long vec_reve (vector signed long long);
vector unsigned long long vec_reve (vector unsigned long long);
vector bool short vec_reve (vector bool short);
vector signed short vec_reve (vector signed short);
vector unsigned short vec_reve (vector unsigned short);
vector double vec_reve (vector double);
vector float vec_reve (vector float);

Diff Detail

Event Timeline

jtony updated this revision to Diff 75576.Oct 24 2016, 7:45 AM
jtony retitled this revision from to [PPC] Implement vector reverse elements builtins (vec_reve).
jtony updated this object.
jtony added reviewers: kbarton, amehsan, nemanjai.
jtony added a subscriber: llvm-commits.
jtony updated this revision to Diff 75583.Oct 24 2016, 8:08 AM
jtony changed the visibility from "All Users" to "Public (No Login Required)".
nemanjai added inline comments.Oct 24 2016, 9:16 AM
lib/Headers/altivec.h
15041

I believe the indentation is off on all of these. Please use clang-format or use the plugin/config for your favourite editor (from the utils/ directory). Both of those are ways to enforce the indentation portion of the coding guidelines.

test/CodeGen/builtins-ppc-altivec.c
27

Please do not add conditionally compiled code into the test case. This can lead to confusing results. See my comment below.

9000

Please provide CHECK-LABEL for all the check prefixes and keep the checks upper case (I'm not sure if this actually performs the LABEL check).

9003

Don't check the name of the variable. Just start the check pattern with the instruction (shufflevector).

9042

It would be difficult for an uninitiated reader to decipher what is actually being tested here as a result of this condition.
For example, none of the "CHECK" lines can succeed here because the run commands they correspond to do not include VSX. I'm actually surprised this test case passes.

The functions whose definitions require VSX should go into a separate file in which the run commands will include VSX (for example builtins-ppc-vsx.c).

nemanjai added inline comments.Oct 24 2016, 9:20 AM
test/CodeGen/builtins-ppc-altivec.c
9042

Oh, I see why it's passing. None of these checks are actually performed. FileCheck will perform the check if the line starts with (in this case):
CHECK:
CHECK-LE:

i.e. Notice the colon at the end.

nemanjai requested changes to this revision.Oct 24 2016, 9:21 AM
nemanjai edited edge metadata.
This revision now requires changes to proceed.Oct 24 2016, 9:21 AM
nemanjai added inline comments.Oct 24 2016, 9:28 AM
test/CodeGen/builtins-ppc-altivec.c
9000

I just realized that the existing tests in this file don't check the label for both prefixes. That's fine and you're not expected to change that, but since you're writing a new test, provide both
CHECK-LABEL:
CHECK-LE-LABEL:

amehsan added inline comments.Oct 24 2016, 9:48 AM
test/CodeGen/builtins-ppc-altivec.c
9000

Something that I do sometimes to make sure my test is actually doing something is the following. I change the testcase slightly so it has to fail. Then I check again. If it passes, then something is wrong.

amehsan added inline comments.Oct 24 2016, 9:53 AM
test/CodeGen/builtins-ppc-altivec.c
9000

I think this comment should have been further down in the file :P

jtony updated this revision to Diff 75618.Oct 24 2016, 11:54 AM
jtony edited edge metadata.
jtony marked 9 inline comments as done.

Modify the code according to NEMANJA IVANOVIC and EHSAN AMIRI's comments (fix clang format violation and move some VSX specific source to builtins-ppc-vsx.c etc.) .

jtony added a reviewer: sfertile.

Other than the inline comment, this LGTM but I'll let @kbarton give the official approval so we get another pair of eyes on the patch.

lib/Headers/altivec.h
15041

Sorry I missed this in the first review, but why do we have a temp here? Why not just a return statement to return the result of __builtin_shufflevector? I realize that it won't make much of a difference with the code-gen with optimization, but it is clearer and more readable to just have a return statement since the temp isn't used for anything else.

jtony updated this revision to Diff 75700.Oct 25 2016, 7:41 AM
jtony added a subscriber: echristo.

Remove unnecessary temp variables.

kbarton accepted this revision.Oct 25 2016, 8:45 AM
kbarton edited edge metadata.

LGTM

echristo accepted this revision.Oct 25 2016, 2:26 PM
echristo added a reviewer: echristo.

One small minor thing and then LGTM. Thanks!

test/CodeGen/builtins-ppc-altivec.c
9001

Can you sink these to just inside the function? (Minor preference)

jtony updated this revision to Diff 75879.Oct 26 2016, 7:24 AM
jtony edited edge metadata.
jtony marked 2 inline comments as done.

Sink CHECK-LABEL andCHECK-LE-LABEL to just inside the function test8 according to echristo's comment.

Committed revision 285218 on behalf of Tony.
Tony, please observe the buildbots at http://lab.llvm.org:8011/console to see if you spot any failures. If none, please close this review.

Committed revision 285218 on behalf of Tony.
Tony, please observe the buildbots at http://lab.llvm.org:8011/console to see if you spot any failures. If none, please close this review.

Committed revision 285218 on behalf of Tony.
Tony, please observe the buildbots at http://lab.llvm.org:8011/console to see if you spot any failures. If none, please close this review.

Sure, I will monitor the console result. Thanks!

jtony accepted this revision.Oct 27 2016, 7:23 AM
jtony added a reviewer: jtony.
nemanjai accepted this revision.Oct 27 2016, 7:38 AM
nemanjai edited edge metadata.

This is ready to close now.

This revision is now accepted and ready to land.Oct 27 2016, 7:38 AM
jtony closed this revision.Oct 27 2016, 7:44 AM