Page MenuHomePhabricator

[Power10] Implement Vector Splat Immediate Builtins in LLVM/Clang
ClosedPublic

Authored by biplmish on Wed, Jun 24, 10:35 PM.

Details

Summary

This patch implements builtins for the following prototypes:

vector signed int vec_splati (const signed int);
vector float vec_splati (const float);

vector double vec_splatid (const float);

vector signed int vec_splati_ins (vector signed int, const unsigned int, const signed int);
vector unsigned int vec_splati_ins (vector unsigned int, const unsigned int, const unsigned int);
vector float vec_splati_ins (vector float, const unsigned int, const float);

Diff Detail

Event Timeline

biplmish created this revision.Wed, Jun 24, 10:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jun 24, 10:35 PM
amyk added inline comments.Fri, Jun 26, 11:33 AM
clang/include/clang/Basic/BuiltinsPPC.def
449 ↗(On Diff #273227)

Nit: space after //

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1477 ↗(On Diff #273227)

This PPCISD node is not used anywhere?

llvm/lib/Target/PowerPC/PPCISelLowering.h
100 ↗(On Diff #273227)

Maybe we can say PPC VSX Splat Immediate instructions for converting . . .

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
588 ↗(On Diff #273227)

There's a section for anonymous patterns at the bottom of the file. It may be better to have a separate PrefixInstrs anonymous patterns section there.

biplmish marked 2 inline comments as done.Mon, Jun 29, 4:35 AM
biplmish marked 2 inline comments as done.Mon, Jun 29, 4:37 AM
biplmish added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1477 ↗(On Diff #273227)

This is used in XXSPLTIDP. The input to builtin is a single precision float which needs to be converted to a double precision for the input xxspltidp

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
588 ↗(On Diff #273227)

Can implement on these lines.

What I see from this patch is:

  • builtins -> expressions
  • intrinsics -> hw instructions.

I didn't see any test or something that we can prove that, the expression can be lowered to hw instructions.

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

The test is too weak. Please check <4 x i32><i32 -1, i32 -1, i32 -1, i32 -1>

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
573 ↗(On Diff #273227)

Not prefix instruction.

581 ↗(On Diff #273227)

Can you explain more on why we need this pattern ? I didn't see any test for this .

biplmish marked 2 inline comments as done.Mon, Jun 29, 9:41 PM

On intrinsics -> hw instructions, will have a discussion and if needs to be implemented in these lines.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
573 ↗(On Diff #273227)

These are prefixed 64 bit instructions.

581 ↗(On Diff #273227)

This is used in XXSPLTIDP. The input to builtin is a single precision float which needs to be converted to a double precision for the input xxspltidp

biplmish updated this revision to Diff 274748.Wed, Jul 1, 3:55 AM

Repatching after the p10 vector splat immediate instructions have been merged upstream.

steven.zhang accepted this revision.Thu, Jul 2, 1:25 AM

LGTM now as https://reviews.llvm.org/D82911 landed to exploit the splat hw instruction. Please hold on to see if @lei or @amyk have more comments.

This revision is now accepted and ready to land.Thu, Jul 2, 1:25 AM
nemanjai requested changes to this revision.Thu, Jul 2, 2:59 AM

If this patch does not include the back end codegen test cases for these, which patch does? I don't see any selection patterns or test cases for xxsplti32dx in ToT.
The implementation and test cases for xxspltiw are in ToT, so they don't appear to be a concern.

clang/lib/Headers/altivec.h
16903

Again, I think it is preferable from a readability standpoint to keep the LE and BE blocks close together:

static __inline__ vector signed int __ATTRS_o_ai vec_splati_ins(
    vector signed int __a, const unsigned int __b, const signed int __c) {
  assert(__b < 2 && "The second argument must be 0 or 1");
#ifdef __LITTLE_ENDIAN__
  __a[1 - __b] = __c;
  __a[3 - __b] = __c;
#else
  __a[__b] = __c;
  __a[2 + __b] = __c;
#endif
  return __a;
}

And similarly for the other two. Of course, in this case you can't really forward calls because there isn't really a way to represent a bitcast of a float constant to int in the language.

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

You have separate checks for LE/BE. Please show the index in the checks and don't use the same index for all 3 test cases.

This revision now requires changes to proceed.Thu, Jul 2, 2:59 AM

Also, please add a couple of new test in test/CodeGen/PowerPC/p10-splatImm.ll so that when someone looks at this commit in the log, they can see that vec_splati and vec_splatid are meant to produce xxspltiw and xxspltidp (i.e. the same IR produced for the calls in this test, produces the desired instruction)..

lei added inline comments.Thu, Jul 2, 7:01 AM
clang/test/CodeGen/builtins-ppc-p10vector.c
12

I don't see why this is needed as it's the exact same test as line 2 but with a diff check-prefix.

192

Why bother with different checks for BE and LE? Seems they produce the same outputs.

amyk added inline comments.Mon, Jul 6, 7:56 AM
clang/test/CodeGen/builtins-ppc-p10vector.c
194

Would also be good to add tests where the second argument is 1.

biplmish updated this revision to Diff 275757.Mon, Jul 6, 10:13 AM

Code changes to bring the LE and BE block close, add additional tests.

lei added inline comments.Mon, Jul 6, 10:25 AM
clang/test/CodeGen/builtins-ppc-p10vector.c
535

missing CHECK-BE?

biplmish added inline comments.Mon, Jul 6, 10:50 AM
clang/test/CodeGen/builtins-ppc-p10vector.c
535

There will 1 less case for BE as one splat location will be the same as index.

lei added inline comments.Mon, Jul 6, 11:16 AM
clang/test/CodeGen/builtins-ppc-p10vector.c
517

missing CHECK-BE

521

same

527

same

535

not sure what you mean. There is not checks for BE here at all....

amyk added inline comments.Mon, Jul 6, 11:23 AM
clang/lib/Headers/altivec.h
17109

Move function name on next line for consistency.

17115

Also move function name to next line.

17128

Also move function name to next line.

biplmish marked an inline comment as done.Mon, Jul 6, 11:57 AM
biplmish added inline comments.
clang/lib/Headers/altivec.h
17109

These changes break the clang-format.

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

vec_splati and vec_splati do not have an endianess specific implementation.

biplmish updated this revision to Diff 275790.Mon, Jul 6, 12:14 PM

Update tests to add checks for BE.

lei accepted this revision as: lei.Mon, Jul 6, 1:06 PM

LGTM

amyk accepted this revision as: amyk.Mon, Jul 6, 1:10 PM

Unless Lei and Nemanja have any additional comments, LGTM.

nemanjai accepted this revision.Mon, Jul 6, 3:22 PM

Other than removing the assert, this LGTM.

clang/lib/Headers/altivec.h
17116

Sorry that I didn't really pay close enough attention here previously, but please no asserts in the header.

  1. The user of the header may not include <assert.h>
  2. The compiler should not be injecting asserts into user code.
This revision is now accepted and ready to land.Mon, Jul 6, 3:22 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 6, 6:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript