This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add support for vector bool __int128 for Power10
ClosedPublic

Authored by saghir on Jun 14 2020, 7:01 PM.

Details

Summary

This patch adds support for vector bool __int128 type for Power10.

Diff Detail

Event Timeline

saghir created this revision.Jun 14 2020, 7:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2020, 7:01 PM
amyk added a subscriber: amyk.Jun 15 2020, 1:16 PM
amyk added inline comments.
clang/test/Parser/p10-vector-bool-128.c
2

Just curious but does the RUN line require pwr10?

lei requested changes to this revision.Jun 16 2020, 8:37 AM
lei added a subscriber: lei.
lei added inline comments.
clang/lib/Sema/DeclSpec.cpp
1154

Not sure what you mean here.

1166

I think you should only check for "power10-vector" since this requires pwr10?

clang/test/Parser/altivec-bool-128.c
3

test for -mcpu=pwr10 -target-feature -power10-vector and -mcpu=pwr10 -target-feature -vsx
same for cxx-altivec-bool-128.cpp

clang/test/Parser/p10-vector-bool-128.c
3

add run line for feature cpu=pwr10 +power10-vector

This revision now requires changes to proceed.Jun 16 2020, 8:37 AM
saghir updated this revision to Diff 271137.Jun 16 2020, 10:38 AM
saghir marked 4 inline comments as done.Jun 16 2020, 10:53 AM
saghir added inline comments.
clang/lib/Sema/DeclSpec.cpp
1154

Earlier comment had the Programming Interface Manual version number (PIM 2.1); asserting only char/int were valid with vector bool.
This patch adds __int128, so the comment needs to be updated with the latest version of the document once it is available.

clang/test/Parser/altivec-bool-128.c
3

This test basically checks that VSX needs to be enabled to have vector bool __int128 type work.

clang/test/Parser/p10-vector-bool-128.c
2

Added pwr10.

3

Added pwr10.
vector bool __int128 type should work with pwr10 and vsx enabled, power10-vector is not needed explicitly.

lei added inline comments.Jun 16 2020, 11:09 AM
clang/test/Parser/altivec-bool-128.c
3

Yes. I am asking you to add a test to check that power10-vector is also needed to be enabled for this type to work.
eg. this type should not be enabled for -mcpu=pwr10 -target-feature +vsx -target-feature -power10-vector

clang/test/Parser/p10-vector-bool-128.c
3

cpu=pwr10 -vsx +power10-vector

saghir updated this revision to Diff 271144.Jun 16 2020, 11:15 AM

Addressed comments.

saghir marked 2 inline comments as done.Jun 16 2020, 11:16 AM
lei added inline comments.Jun 16 2020, 11:37 AM
clang/lib/Sema/DeclSpec.cpp
1168

Do we not need to add a check for int128 to this check as well?

lei added inline comments.Jun 16 2020, 11:39 AM
clang/lib/Sema/DeclSpec.cpp
1154

Maybe need to add a TODO in all the sections of code, that you update, where it mentions PIM 2.1

saghir updated this revision to Diff 271164.Jun 16 2020, 12:19 PM

Updated comment for allowing __int128 for vector bool.

saghir updated this revision to Diff 271459.Jun 17 2020, 1:00 PM
saghir marked 2 inline comments as done.

Updated tests.

saghir marked 3 inline comments as done.Jun 17 2020, 1:09 PM
saghir added inline comments.
clang/lib/Sema/DeclSpec.cpp
1154

I have changed comment such that it does not need the document version.

clang/test/Parser/altivec-bool-128.c
3

Done, added a line for pwr10, +vsx and -power10-vector.

clang/test/Parser/p10-vector-bool-128.c
3

I have now added both -target-cpu pwr10 and -target-feature +power10-vector as you mentioned in the first comment.
However, I am not quite sure what you are looking to test here by adding -vsx because that would disable vsx, which in turn would disable power10-vector and we would not be able to test the legitimate use here.

saghir marked 2 inline comments as done.Jun 17 2020, 1:14 PM
saghir added inline comments.
clang/lib/Sema/DeclSpec.cpp
1168

No need here because this checks for type TST_int only.

lei added inline comments.Jun 17 2020, 1:41 PM
clang/test/Parser/p10-vector-bool-128.c
3

-vsx diable it, but the following +power10-vector should enable it.
So:

// RUN: %clang_cc1 -triple=powerpc64le-unknown-linux-gnu -target-cpu pwr10 \
// RUN:            -target-feature -vsx -target-feature +power10-vector
3

I don't think you need -target-feature +altivec here.

6

this line shouldn't be needed.

saghir updated this revision to Diff 271498.Jun 17 2020, 3:13 PM
saghir marked an inline comment as done.

Updated tests features to check.

saghir marked 2 inline comments as done.Jun 17 2020, 3:15 PM
saghir added inline comments.
clang/test/Parser/p10-vector-bool-128.c
3

Sorry, I did not accurately explain what happens if we disable vsx.
We cannot enable power10-vector and disable vsx at the same time, well we can but the compiler would give the following error:
error: option '-mpower10-vector' cannot be specified with '-mno-vsx'

ppcUserFeaturesCheck function in PPC.cpp ensures that any vsx dependant option cannot be turned on while simultaneously disabling vsx. So for this reason the above test line that you mentioned would not work.

saghir marked an inline comment as done.Jun 18 2020, 10:28 AM
saghir updated this revision to Diff 271860.Jun 18 2020, 3:36 PM
saghir edited the summary of this revision. (Show Details)

Updated comment to reflect the change in allowed types.

saghir updated this revision to Diff 271874.Jun 18 2020, 4:27 PM
saghir added a reviewer: stefanp.

Updated comment.

amyk accepted this revision as: amyk.Jun 22 2020, 2:58 PM

Other than Lei's concerns, personally I think this looks good to me.

clang/test/Parser/p10-vector-bool-128.c
6

I believe this comment still needs to be addressed (of the line not being needed).

saghir marked 2 inline comments as done.Jun 22 2020, 3:46 PM
saghir added inline comments.
clang/test/Parser/p10-vector-bool-128.c
6

I think this was for a line removed earlier.

lei accepted this revision as: lei.Jun 23 2020, 6:07 AM

LGTM, please address the 1 comment I have on commit.

clang/test/Parser/altivec-bool-128.c
6

I don't think this is needed here -target-feature +altivec

lei accepted this revision.Jun 23 2020, 6:07 AM
This revision is now accepted and ready to land.Jun 23 2020, 6:07 AM
This revision was automatically updated to reflect the committed changes.
saghir marked an inline comment as done.
saghir marked an inline comment as done.Jun 23 2020, 8:00 PM