[PPC] Add vec_insert4b/vec_extract4b to altivec.h

[sfertile]

Summary

Add macros that implement the vec_extract4b and vec_insert4b functionality.

vector unsigned long long vec_extract4b (vector unsigned char, const int)
Purpose:
Extracts a word from a vector at a byte position.
Result value:
The first doubleword element of the result contains the zero-extended extracted word from ARG1. The second doubleword is set to 0. ARG2 specifies the least-significant byte number (0 - 12) of the word to be extracted

vector unsigned char vec_insert4b (vector signed int, vector unsigned char, const int)
vector unsigned char vec_insert4b (vector unsigned int, vector unsigned char, const int)
Purpose:
Inserts a word into a vector at a byte position.
Result Value:
The contents of word element 1 of the first argument are extracted and placed into argument 2. The word is inserted into argument 2 starting at the byte offset indicated by the third argument.

[nemanjai]

lib/Headers/altivec.h

12014

As far as I can tell by looking at this patch and the corresponding back end patch, the __a argument will have a word inserted into it and it will be returned.

Is that the semantics that the ABI specifies (I can't seem to make sense of the description).

vector unsigned int a = { 0xAAAAAAAA, 0xBBBBBB, 0xCCCCCC, 0xDDDDDD };
vector unsigned char b = (vector unsigned char) 0xFF;
vector unsigned char c = vec_insert4b(a, b, 4);
// Do we expect vector c to be:
// { 0xAA, 0xAA, 0xAA, 0xAA, 0xFF, 0xFF, 0xFF, 0xFF, 0xCC, 0xCC, 0xCC, 0xCC, 0xDD, 0xDD, 0xDD, 0xDD }

[kbarton]

lib/Headers/altivec.h

12014

I think the current version of the ABI document has an error in it. The description of the vec_insert4b is identical to the vec_extract4b, so I expect it was copy/pasted in error. I think we need to open up an (internal) bug against the ABI and wait for clarification to complete this.

[nemanjai]

lib/Headers/altivec.h

11908

I find it difficult to follow and understand this logic when it's in the header.
What I'd prefer to see here is that the macro simply expands into __builtin_vsx_xxextractuw and then handle all this logic in the code that emits an intrinsic call.
Namely if the target is little endian, we adjust the parameter, emit the intrinsic call and finally emit a shufflevector.

[sfertile]

lib/Headers/altivec.h

11908

I think this is a good idea, looking at the code its not obvious what is going on.

12014

You have it correct Nemanja, word 1 will be extracted from b, and it will get inserted into a. The word will be inserted at the byte position starting at the 3rd argument. (so in this case byte offsets 4 to 7)

I talked to Bill Schmidt earlier today and he already has a bug open. It hasn't been updated yet, but it should roughly correspond to the description for the xxinsertw instruction:

The contents of word element 1 of VSR[XB] are placed
into byte elements UIM:UIM+3 of VSR[XT]. The contents
of the remaining byte elements of VSR[XT] are not
modified.

[kbarton]

Did you upload a new patch? The diff doesn't appear to change for me.

[sfertile]

Moved the endian related massaging from altivec.h into Clang codegen and clamped the input index into the valid range [0, 12].

[syzaara]

lib/CodeGen/CGBuiltin.cpp

8185 ↗

(On Diff #78760)

tiny comment, char is misspelled as cahr

[sfertile]

Fixed spelling error in comment

[sfertile]

lib/CodeGen/CGBuiltin.cpp

8185 ↗

(On Diff #78760)

Fixed

[nemanjai]

lib/CodeGen/CGBuiltin.cpp

8182 ↗

(On Diff #78911)

assert(ArgCI && "The third operand of this intrinsic must be a compile-time constant")

8183 ↗

(On Diff #78911)

s/index/Index

8191 ↗

(On Diff #78911)

Just a minor nit, but I'd initialize this inline here.

8213 ↗

(On Diff #78911)

Same comment as above - make sure your asserts tell the developer why they trip.

test/CodeGen/builtins-ppc-p9vector.c

831

Add testing with an argument out of range as well as non-constant (presumably in a separate test case for the latter).

[kbarton]

LGTM, but I'd like @nemanjai to have a quick look at the builtin_vsx_insertword and builtin_vsx_extractuword too.

[nemanjai]

Overall, I'd like to see another revision of this. This is not because I feel the patch requires major rework, but because there is a number of minor comments that I'd like to make sure are all addressed.
Just a few notes:

• Please be careful with the naming convention (mentioned in inline comments)
• Be descriptive with your asserts - imagine being a developer that gets an assert message that just says ArgCI - it would probably not mean much to you.
• I am pretty sure this does not require any work in Sema since AFAICT the builtins are defined to require that the third parameter is a constant (i.e. Ii rather than just i). But this should be tested (mentioned in inline comments).
• I would like to see testing cover all of the code you generate (i.e. you generate bitcasts to <2 x i64> and I don't see it in the tests)
• I would like to see early exits if the subtarget does not have P9Vector. This is kind of subtle, but for most builtins that lower to an intrinsic that requires some subtarget feature that isn't present, the user gets a not able to compile this builtin yet. As you have it implemented, it would not be the case here - it would lower to an intrinsic and we'd get an ISEL error. I think the latter situation is inferior so we should find a way to get the former. I am hoping this will not require Sema (I'm not sure if returning a nullptr will achieve this goal but hopefully something along those lines will). Basically, the point is that you shouldn't assume that the only way you'd get a builtin call is through the interface defined in the ABI, but that someone may actually insert a __builtin_<whatever> in their code.

lib/CodeGen/CGBuiltin.cpp

39 ↗

(On Diff #78911)

This nit applies to all naming in this changeset: variables start with an uppercase letter and functions with a lowercase letter.

8194 ↗

(On Diff #78911)

Minor nit: please settle on where the star goes when declaring pointers.

8198 ↗

(On Diff #78911)

Just a general nit here - we prefer comments to be complete sentences with capitalization and punctuation.

test/CodeGen/builtins-ppc-p9vector.c

836

How does this pass? The "T2" is not enclosed in double square brackets.

[sfertile]

Changed all variable names to start with capitals, added description strings to the assertions, changed uses of '12' to MaxIndex and initialized the arrays in their definition.

[nemanjai]

lib/CodeGen/CGBuiltin.cpp

8193 ↗

(On Diff #79860)

This will likely have to change when the wording is settled on for this builtin. Namely, the element to insert is in BE word element 1 for the instruction. So it is quite likely that the builtin will be made "endianness-neutral" by specifying the source element to be word element 1 (for both LE/BE).

[nemanjai]

The finalized wording for vec_insert4b:
Let W be the first doubleword element of ARG1, truncated to 32 bits. The result vector is formed by inserting W into ARG2 at the byte position (0-12) specified by ARG3.

Given that wording, this implementation has almost the correct semantics. The only change required is that the source and target vectors need to be switched. Namely, a word is taken out of the first argument and inserted into the second argument. And it appears that the current implementation has that backwards.

[sfertile]

Updated to swap the arguments when generating the intrinsic. Updated a number of the comments, and added some tests with the index out of range.

[nemanjai]

Just one minor inline nit. Other than that, LGTM.

test/CodeGen/builtins-ppc-error.c

5 ↗

(On Diff #82031)

I'm just curious why -Werror? This should be an error even without that.

[nemanjai]

test/CodeGen/builtins-ppc-extractword-error.c

2 ↗

(On Diff #82031)

I think this will fail on all the powerpc targets, such as powerpc64le, etc. Which isn't what you want I imagine.
Also, I think this isn't really the idea with XFAIL. The semantics we'd probably want are better served by using the not tool and checking for the error messages. You should be able to find some example of how the not tool is used in some of the other test cases (for example test/CodeGen/builtins-ppc-p7-disabled.c).

[echristo]

This was committed:

commit d65cd1f9424369c4ae7f945fac7fd9e4357451b2
Author: Sean Fertile <[e-mail]>
Date: Thu Jan 5 21:43:30 2017 +0000

Add vec_insert4b and vec_extract4b functions to altivec.h

Add builtins for the functions and custom codegen mapping the builtins to their
corresponding intrinsics and handling the endian related swapping.

https://reviews.llvm.org/D26546

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@291179 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/BuiltinsPPC.def

 BUILTIN(__builtin_vsx_xvcpsgnsp, "V4fV4fV4f", "")
 
 BUILTIN(__builtin_vsx_xvabssp, "V4fV4f", "")
 BUILTIN(__builtin_vsx_xvabsdp, "V2dV2d", "")
 
 BUILTIN(__builtin_vsx_xviexpdp, "V2dV2ULLiV2ULLi", "")
 BUILTIN(__builtin_vsx_xviexpsp, "V4fV4UiV4Ui", "")
 
+BUILTIN(__builtin_vsx_xxinsertw, "V16UcV4UiV16UcIi", "")
+BUILTIN(__builtin_vsx_xxextractuw, "V2ULLiV16UcIi", "")
+
 // HTM builtins
 BUILTIN(__builtin_tbegin, "UiUIi", "")
 BUILTIN(__builtin_tend, "UiUIi", "")
 
 BUILTIN(__builtin_tabort, "UiUi", "")
 BUILTIN(__builtin_tabortdc, "UiUiUiUi", "")
 BUILTIN(__builtin_tabortdci, "UiUiUii", "")
 BUILTIN(__builtin_tabortwc, "UiUiUiUi", "")

lib/Headers/altivec.h

static __inline__ double __ATTRS_o_ai vec_extract(vector double __a, int __b) {
   return __a[__b];
 }
 #endif
 
 static __inline__ float __ATTRS_o_ai vec_extract(vector float __a, int __b) {
   return __a[__b];
 }
 
+#if defined(__POWER9_VECTOR__)
+#ifdef __LITTLE_ENDIAN__
+#define vec_extract4b(__a, __b)                                                \
+   vec_reve((vector unsigned long long)                                        \
+                __builtin_vsx_xxextractuw((__a), (12 - (__b & 0xF))))
+#else
+#define vec_extract4b(__a, __b)  __builtin_vsx_xxextractuw((__a), (__b))
+#endif
+#endif /* if defined(__POWER9_VECTOR__) */
+
 /* vec_insert */
 
 static __inline__ vector signed char __ATTRS_o_ai
 vec_insert(signed char __a, vector signed char __b, int __c) {
   __b[__c] = __a;
   return __b;
 }
 

 
 static __inline__ vector float __ATTRS_o_ai vec_insert(float __a,
                                                        vector float __b,
                                                        int __c) {
   __b[__c] = __a;
   return __b;
 }
 
+#if defined(__POWER9_VECTOR__)
+#ifdef __LITTLE_ENDIAN__
+#define vec_insert4b(__a, __b, __c)                                            \
+  ((vector unsigned char)__builtin_vsx_xxinsertw((__a),                        \
+                                   vec_reve((vector unsigned long long)(__b)), \
+                                   12 - ((__c) & 0xF)))
+#else
+#define vec_insert4b(__a, __b, __c) \
+  ((vector unsigned char)__builtin_vsx_xxinsertw((__a), (__b), (__c) & 0xF))
+#endif
+#endif /* if defined(__POWER9_VECTOR__) */
+
 /* vec_lvlx */
 
 static __inline__ vector signed char __ATTRS_o_ai
 vec_lvlx(int __a, const signed char *__b) {
   return vec_perm(vec_ld(__a, __b), (vector signed char)(0),
                   vec_lvsl(__a, __b));
 }
 

test/CodeGen/builtins-ppc-p9vector.c

 }
 vector unsigned char test73(void) {
 // CHECK-BE: @llvm.ppc.altivec.vsrv(<16 x i8>
 // CHECK-BE-NEXT: ret <16 x i8>
 // CHECK: @llvm.ppc.altivec.vsrv(<16 x i8>
 // CHECK-NEXT: ret <16 x i8>
   return vec_srv (vuca, vucb);
 }
-
+vector unsigned char test74(void) {
+// CHECK-BE: [[T1:%.+]] = call <4 x i32> @llvm.ppc.vsx.xxinsertw(<4 x i32> {{.+}}, <4 x i32> {{.+}}, i32 5)
+// CHECK-BE-NEXT: bitcast <4 x i32> [[T1]] to <16 x i8>
+// CHECK: [[T1:%.+]] = call <4 x i32> @llvm.ppc.vsx.xxinsertw(<4 x i32> {{.+}}, <4 x i32> {{.+}}, i32 7)
+// CHECK-NEXT: bitcast <4 x i32> [[T1]] to <16 x i8>
+  return vec_insert4b(vuia, vuca, 5);
+}
+vector unsigned char test75(void) {
+// CHECK-BE: [[T1:%.+]] = call <4 x i32> @llvm.ppc.vsx.xxinsertw(<4 x i32> {{.+}}, <4 x i32> {{.+}}, i32 5)
+// CHECK-BE-NEXT: bitcast <4 x i32> [[T1]] to <16 x i8>
+// CHECK: [[T1:%.+]] = call <4 x i32> @llvm.ppc.vsx.xxinsertw(<4 x i32> {{.+}}, <4 x i32> {{.+}}, i32 7)
+// CHECK-NEXT: bitcast <4 x i32> [[T1]] to <16 x i8>
+  return vec_insert4b(vsia, vuca, 5);
+}
+vector unsigned long long test76(void) {
+// CHECK-BE: [[T1:%.+]] = call <2 x i64> @llvm.ppc.vsx.xxextractuw(<4 x i32> {{.+}}, i32 11)
+// CHECK-BE-NEXT: ret <2 x i64> [[T1]]
+// CHECK: call <2 x i64> @llvm.ppc.vsx.xxextractuw(<4 x i32> {{.+}}, i32 1)
+// CHECK: [[T1:%.+]] = shufflevector <2 x i64> {{.+}}, <2 x i64> {{.+}}, <2 x i32> <i32 1, i32 0>
+// CHECK-NEXT: ret <2 x i64> [[T1]]
+  return vec_extract4b(vuca, 11);
+}