This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Power10] Implemented Vector Shift Builtins
ClosedPublic

Authored by Conanap on Jul 7 2020, 12:36 PM.

Details

Reviewers
saghir
nemanjai
hfinkel
amyk
Group Reviewers
Restricted Project
Commits
rG3136cbe29e74: [PowerPC] Implement Vector Shift Builtins
Summary

Implemented Builtins for vector right and left shift along with appropriate test cases for ISA3.1. Depends on D83516.

Diff Detail

Event Timeline

Conanap created this revision.Jul 7 2020, 12:36 PM

Shouldn't we have test cases to test vec_sl, vec_sr and vec_sra ?

llvm/include/llvm/IR/IntrinsicsPowerPC.td
800 ↗(On Diff #276179)

nit: indentation issue

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1009

nit: extra spaces before : here and in the next two lines

amyk requested changes to this revision.Jul 9 2020, 3:43 PM
amyk added a subscriber: amyk.

This will need to be rebased against your 2608 instruction definitions patch.
But yes, I believe you are missing the clang and llc test case for this patch. Requesting changes due to missing tests.

clang/lib/Headers/altivec.h
17217

/* vs[l | r | raq] */ (with a new line after the comment)

This revision now requires changes to proceed.Jul 9 2020, 3:43 PM
Conanap updated this revision to Diff 277896.Jul 14 2020, 10:42 AM
Conanap marked 3 inline comments as done.

Added tests, added extra comments.

Conanap edited the summary of this revision. (Show Details)Jul 14 2020, 10:42 AM

Also removed instr def as it will be part of D83516.

amyk added inline comments.Jul 14 2020, 11:04 AM
clang/lib/Headers/altivec.h
17217

I think we can remove /* vector shifts for quadwords */.
Then, we can add a new line after /* vs[l | r | raq] */ for consistency of the comments.

clang/test/CodeGen/builtins-ppc-p10vector.c
586

vector unsigned you mean? For the return of this test and every other unsigned test case.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1113–1114

nit: remove extra space after <

1115

nit: remove after space before ,

1121

Unrelated space change?

llvm/test/CodeGen/PowerPC/p10-vector-shift.ll
10 ↗(On Diff #277896)

Can probably remove these globals as you are not using them in your test.

amyk added inline comments.Jul 14 2020, 11:19 AM
clang/lib/Headers/altivec.h
17219

I believe there are supposed to be signed and unsigned versions of the functions? It looks like you currently have all of them as unsigned.

Conanap updated this revision to Diff 278235.Jul 15 2020, 10:17 AM
Conanap marked 7 inline comments as done.

Formatting fixes, fixed test case return type, updated builtins' signatures to correct signatures.

stefanp added a subscriber: stefanp.EditedJul 16 2020, 9:14 AM

Just a few nits for this patch.

Edit: Please wait for Amy to give the approval.

clang/lib/Headers/altivec.h
17224

nit:
Is this supposed to be vec_slq?

llvm/test/CodeGen/PowerPC/p10-vector-shift.ll
10 ↗(On Diff #278235)

nit:
If you are going to use #0 you can probably define attributes #0 = { nounwind } at the bottom of this file.

41 ↗(On Diff #278235)

nit:
You probably don't need the #1 as it is not defined anyway.

Overall seems fine to me, but of course, please wait to hear from Amy.
Just some nits for the test case.

llvm/test/CodeGen/PowerPC/p10-vector-shift.ll
9 ↗(On Diff #278235)

nit: please add a description for test purpose.

10 ↗(On Diff #278235)

nit: The test name and CHECK-LABEL should use test_vec_vslq

10 ↗(On Diff #278235)

+1, if no specific attributes needed. please remove all the #0 and #1

amyk added a comment.Jul 17 2020, 2:27 PM

Please also change the function names.

clang/lib/Headers/altivec.h
17217

Actually, sorry, I think this comment should be the following instead:
/* vec_s[l | r | ra] */ since these functions are actually supposed to be vec_sl, vec_sr, vec_sra.

17224

Actually vec_sl seems to be correct. However, that would mean the other functions need to be renamed as the functions are supposed to be: vec_sl, vec_sr, vec_sra. Albion, could you please rename these functions and also ensure your test uses the correct naming.

Conanap updated this revision to Diff 279348.Jul 20 2020, 2:38 PM
Conanap marked 8 inline comments as done.

Fixed function names, test case clean up

amyk added a comment.Jul 24 2020, 8:50 AM

I realize it may be possible to open code these, as these functions already exist in altivec.h. Could you look into if this is the case?

Conanap updated this revision to Diff 280592.Jul 24 2020, 2:44 PM
Conanap removed a reviewer: power-llvm-team.

Converted the impelmentation to an open coded implementation and updated the test cases as appropriate.

lei added a subscriber: lei.Jul 27 2020, 6:38 AM

Please address the auto generated clang-format issues for the added code in this patch.

Conanap edited the summary of this revision. (Show Details)Jul 27 2020, 1:09 PM
Conanap updated this revision to Diff 281735.Jul 29 2020, 2:04 PM

Fixed formatting based on bot feedback.

Conanap updated this revision to Diff 281783.Jul 29 2020, 7:37 PM

Replaced a forgotten test file.

amyk added inline comments.Aug 4 2020, 7:33 AM
clang/lib/Headers/altivec.h
17217

Add a space after this comment.

17227

Please remove any commented code.

17243

Could you please fix the indentation of the returns to make them all consistent?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1100

No brackets are needed here.

Also, I think it might make sense to move this block into the previous hasP9Altivec condition since in there it has:

setOperationAction(ISD::SHL, MVT::v1i128, Legal);
setOperationAction(ISD::SRL, MVT::v1i128, Legal);
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1141

There is no need to make a new predicate block, you can put these anonymous patterns in the block above.

1144

I noticed that we have patterns for the PPCISD nodes, but I think no tests to show these patterns?

amyk added inline comments.Aug 5 2020, 12:22 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1100

Oops, I made that comment above since I thought you were putting the block inside a hasP9Altivec block. Sorry.

Maybe you can move this condition right underneath the hasP9Altivec block that contains the SHL/SRL to be closer to those instructions.

amyk added inline comments.Aug 6 2020, 10:41 PM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
1144

My apologies, upon closer inspection I noticed that your tests are in fact the ones with the PPCISD nodes but you are missing the regular shl/srl/sra. Please add the tests for these, as well.

Conanap updated this revision to Diff 283980.Aug 7 2020, 12:13 PM
Conanap marked 8 inline comments as done.

Added shl tests, formatting fixes

clang/lib/Headers/altivec.h
17243

I ran clang-format this time, lmk if we should change this as it doesn't look like clang-format was too consistent with itself

amyk accepted this revision.Aug 9 2020, 9:04 PM

Thanks for addressing the comments. LGTM.

llvm/test/CodeGen/PowerPC/p10-vector-shift.ll
10 ↗(On Diff #283980)

nit:

These test cases demonstrate that the vector shift quadword instructions introduced 
within Power10 are correctly exploited.
This revision is now accepted and ready to land.Aug 9 2020, 9:04 PM
This revision was landed with ongoing or failed builds.Aug 12 2020, 4:27 PM
This revision was automatically updated to reflect the committed changes.