This is an archive of the discontinued LLVM Phabricator instance.

[Power10] Implement Vector Shift Double Bit Immediate Builtins in LLVM/Clang
ClosedPublic

Authored by biplmish on Jun 24 2020, 1:54 AM.

Details

Summary

This patch implements builtins for the following prototypes:

vector signed char vec_sldb (vector signed char, vector signed char, const unsigned int);
vector unsigned char vec_sldb (vector unsigned char, vector unsigned char, const unsigned int);
vector signed short vec_sldb (vector signed short, vector signed short, const unsigned int);
vector unsigned short vec_sldb (vector unsigned short, vector unsigned short, const unsigned int);
vector signed int vec_sldb (vector signed int, vector signed int, const unsigned int);
vector unsigned int vec_sldb (vectoextracthr unsigned int, vector unsigned int, const unsigned int);
vector signed long long vec_sldb (vector signed long long, vector signed long long, const unsigned int);
vector unsigned long long vec_sldb (vector unsigned long long, vector unsigned long long, const unsigned int);

vector signed char vec_srdb (vector signed char, vector signed char, const unsigned int);
vector unsigned char vec_srdb (vector unsigned char, vector unsigned char, const unsigned int);
vector signed short vec_srdb (vector signed short, vector signed short, const unsigned int);
vector unsigned short vec_srdb (vector unsigned short, vector unsigned short, const unsigned int);
vector signed int vec_srdb (vector signed int, vector signed int, const unsigned int);
vector unsigned int vec_srdb (vector unsigned int, vector unsigned int, const unsigned int);
vector signed long long vec_srdb (vector signed long long, vector signed long long, const unsigned int);extracth
vector unsigned long long vec_srdb (vector unsigned long long, vector unsigned long long, const unsigned int);

Diff Detail

Event Timeline

biplmish created this revision.Jun 24 2020, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2020, 1:54 AM
amyk requested changes to this revision.Jun 24 2020, 3:35 PM
amyk added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
144

SH can be any integer value between 0 and 7. It is better to add some checking to make sure the immediate is between these values.

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

Nit: Remove the extra space before the .

824

The second value (ps) should not be 0. It should be 1, right?

This revision now requires changes to proceed.Jun 24 2020, 3:35 PM
biplmish added inline comments.Jun 24 2020, 6:51 PM
clang/test/CodeGen/builtins-ppc-p10vector.c
144

I dont see the shift bounds specified. Can you confirm.

biplmish added inline comments.Jun 24 2020, 6:55 PM
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
824

Thats right. Thanks for pointing.

biplmish marked an inline comment as done.Jun 24 2020, 8:16 PM
biplmish added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
144

So while the user input does need bound, the instruction expects values in range 0-7. So we can bound the user and also add a check.

biplmish updated this revision to Diff 273222.Jun 24 2020, 8:18 PM

Incorporated review comments from Amy.

Are these a form of funnel shifts? https://llvm.org/docs/LangRef.html#llvm-fshl-intrinsic

If so would it make sense to emit generic @llvm.fshl.v16i8 style intrinsics to improve optimization opportunities?

Are these a form of funnel shifts? https://llvm.org/docs/LangRef.html#llvm-fshl-intrinsic

If so would it make sense to emit generic @llvm.fshl.v16i8 style intrinsics to improve optimization opportunities?

The concat is common, however the funnel shifts and in general most vector operations occur on each elements of the vector.
However this shift(Vector Shift Double Bit Immediate) is to implement shift of the entire concatenated vector by an immediate shift amount.

Are these a form of funnel shifts? https://llvm.org/docs/LangRef.html#llvm-fshl-intrinsic

If so would it make sense to emit generic @llvm.fshl.v16i8 style intrinsics to improve optimization opportunities?

The concat is common, however the funnel shifts and in general most vector operations occur on each elements of the vector.
However this shift(Vector Shift Double Bit Immediate) is to implement shift of the entire concatenated vector by an immediate shift amount.

np- cheers.

amyk added inline comments.Jun 26 2020, 11:18 AM
clang/lib/Headers/altivec.h
16871

Please align the \ at the 80 char line limit like other functions.

llvm/test/MC/PowerPC/p10.s
36 ↗(On Diff #273222)

Can you please adjust the indenting of the # encoding to be aligned with other lines?

Noted. These can be handled during commit. Alternatively if there is a need to update the diff I can generate the same.

biplmish updated this revision to Diff 274681.Jun 30 2020, 9:21 PM

Repatching after the p10 vector shift double bit immediate instructions have been merged upstream.

nemanjai accepted this revision.Jul 1 2020, 6:07 AM

The nits can be addressed on commit. I don't think another review is required. LGTM.

clang/lib/Headers/altivec.h
16887

Do we need the macro overloading since nothing changes about the invocation? Can we not just:

#define vec_sldb(__a, __b, __c) \
  __builtin_altivec_vsldbi(__a, __b, (__c & 0x7))
clang/test/CodeGen/builtins-ppc-p10vector.c
156

Maybe change one of these to pass a value larger than 7 to vec_sldb so that the truncation is shown.

lei requested changes to this revision.Jul 1 2020, 6:14 AM

Please use similar naming for all new builtin test files:
p10-permute-ops.ll => builtins-ppc-p10permute.ll

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

nit: indent to align to ( in the previous line.

743

nit: this should align with int_ppc... in previous line.

749

same.

750

same.

This revision now requires changes to proceed.Jul 1 2020, 6:14 AM
lei accepted this revision as: lei.Jul 1 2020, 6:30 AM

Please address nits and file name changes on commit.

amyk accepted this revision.Jul 1 2020, 7:02 AM

Thanks for fixing, LGTM but please address other comments on the commit.

This revision is now accepted and ready to land.Jul 1 2020, 7:02 AM
biplmish updated this revision to Diff 274817.EditedJul 1 2020, 8:04 AM

Address indentation issue and rename test file

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 6:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript