This is an archive of the discontinued LLVM Phabricator instance.

[Power10] Implement Vector Insert Builtins in LLVM/Clang
ClosedPublic

Authored by biplmish on Jun 23 2020, 3:25 AM.

Details

Summary

This patch implements builtins for the following prototypes:

vector unsigned char vec_insertl (unsigned char, vector unsigned char, unsigned int);
vector unsigned short vec_insertl (unsigned short, vector unsigned short, unsigned int);
vector unsigned int vec_insertl (unsigned int, vector unsigned int, unsigned int);
vector unsigned long long vec_insertl (unsigned long long, vector unsigned long long, unsigned int);
vector unsigned char vec_insertl (vector unsigned char, vector unsigned char, unsigned int;
vector unsigned short vec_insertl (vector unsigned short, vector unsigned short, unsigned int);
vector unsigned int vec_insertl (vector unsigned int, vector unsigned int, unsigned int);

vector unsigned char vec_inserth (unsigned char, vector unsigned char, unsigned int);
vector unsigned short vec_inserth (unsigned short, vector unsigned short, unsigned int);
vector unsigned int vec_inserth (unsigned int, vector unsigned int, unsigned int);
vector unsigned long long vec_inserth (unsigned long long, vector unsigned long long, unsigned int);
vector unsigned char vec_inserth (vector unsigned char, vector unsigned char, unsigned int);
vector unsigned short vec_inserth (vector unsigned short, vector unsigned short, unsigned int);
vector unsigned int vec_inserth (vector unsigned int, vector unsigned int, unsigned int);

Diff Detail

Event Timeline

biplmish created this revision.Jun 23 2020, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2020, 3:25 AM
lei added a subscriber: lei.Jun 23 2020, 7:48 AM
lei added inline comments.
clang/lib/Headers/altivec.h
16863

vec_insertl( should be on the next line. Same as previous definitions.
Same comment for all the def below this one.

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

Please match the indentation of these new instructions to the ones above and below them.

560

nit: this line should match right under int_ppc_altivec_vinsbvlx.

amyk added inline comments.Jun 23 2020, 8:34 AM
clang/include/clang/Basic/BuiltinsPPC.def
309

End with a period.

llvm/include/llvm/IR/IntrinsicsPowerPC.td
429

Nit: // P10 Vector Insert.

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

The indentation of VINSBLX and instructions below looks okay to me, but move the i64:$rA . . . sections below (int_ppc_altivec_ . . .

biplmish marked 3 inline comments as done.Jun 23 2020, 11:48 PM
biplmish added inline comments.
clang/lib/Headers/altivec.h
16863

This is based on the clang-format update. Changes to this gives warnings from clang-format.

biplmish updated this revision to Diff 273220.Jun 24 2020, 8:00 PM

Review comments incorporated.

biplmish updated this revision to Diff 275303.Jul 3 2020, 12:14 AM

Update the builtin implementation to incorporate instruction changes.

lei added inline comments.Jul 3 2020, 6:24 AM
clang/lib/Headers/altivec.h
16862

consistency. bring vec_insert.. to next line same as previous def.
same for all such below.

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

all these indentation is off by 1.

amyk accepted this revision as: amyk.Jul 3 2020, 9:48 AM

I think aside from Lei's indentation nits, I think LGTM.

This revision is now accepted and ready to land.Jul 3 2020, 9:48 AM
lei added a comment.Jul 3 2020, 10:43 AM

I will commit this for Biplob since he does't have commit access yet.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 1:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lei added inline comments.Jul 3 2020, 2:03 PM
clang/test/CodeGen/builtins-ppc-p10vector.c
12

I just noticed this. There is no need to add this RUN line since it's the same as the one on line 2. Please post a patch to remove this and update tests to use the default CHECK.

biplmish marked an inline comment as done.Jul 3 2020, 11:23 PM
biplmish added inline comments.
clang/test/CodeGen/builtins-ppc-p10vector.c
12

Sure. However there are also tests in Line 125,133 etc which would need modification.

Can we also do "RUN: -o - | FileCheck %s -check-prefixes=CHECK,CHECK-LE" in the test1 and remove the test3 so that the tests work in the current format.

lei added inline comments.Jul 6 2020, 6:42 AM
clang/test/CodeGen/builtins-ppc-p10vector.c
12

Please make all the necessary modification all affected testcases to use CHECK instead of CHECK-LE.