[PATCH] D25906: [PPC] Implement vector reverse elements builtins (vec_reve)

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Headers/altivec.h:15041
+vec_reve(vector bool char __a) {
+  vector bool char __vec = __builtin_shufflevector(__a, __a,
+    15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0);
----------------
I believe the indentation is off on all of these. Please use clang-format or use the plugin/config for your favourite editor (from the utils/ directory). Both of those are ways to enforce the indentation portion of the coding guidelines.


================
Comment at: test/CodeGen/builtins-ppc-altivec.c:9003
+  res_vbc = vec_reve(vbc);
+ // CHECK: %shuffle.{{.+}} = shufflevector <16 x i8> %{{[0-9]+}}, <16 x i8> %{{[0-9]+}}, <16 x i32> <i32 15, i32 14, i32 13, i32 12, i32 11, i32 10, i32 9, i32 8, i32 7, i32 6, i32 5, i32 4, i32 3, i32 2, i32 1, i32 0>
+ // CHECK-LE: %shuffle.{{.+}} = shufflevector <16 x i8> %{{[0-9]+}}, <16 x i8> %{{[0-9]+}}, <16 x i32> <i32 15, i32 14, i32 13, i32 12, i32 11, i32 10, i32 9, i32 8, i32 7, i32 6, i32 5, i32 4, i32 3, i32 2, i32 1, i32 0>
----------------
Don't check the name of the variable. Just start the check pattern with the instruction (shufflevector).


================
Comment at: test/CodeGen/builtins-ppc-altivec.c:27
 
+#ifdef __VSX__
+vector bool long long vbll = {3, 5};
----------------
Please do not add conditionally compiled code into the test case. This can lead to confusing results. See my comment below.


================
Comment at: test/CodeGen/builtins-ppc-altivec.c:9014
+
+// check-label: define void @test8
+void test8() {
----------------
Please provide CHECK-LABEL for all the check prefixes and keep the checks upper case (I'm not sure if this actually performs the LABEL check).


================
Comment at: test/CodeGen/builtins-ppc-altivec.c:9056
+
+ #ifdef __VSX__
+  res_vbll = vec_reve(vbll);
----------------
It would be difficult for an uninitiated reader to decipher what is actually being tested here as a result of this condition.
For example, none of the "CHECK" lines can succeed here because the run commands they correspond to do not include VSX. I'm actually surprised this test case passes.

The functions whose definitions require VSX should go into a separate file in which the run commands will include VSX (for example builtins-ppc-vsx.c).


https://reviews.llvm.org/D25906