[PATCH] D17816: [PPC] FE support for generating VSX [negated] absolute value instructions

[Nemanja Ivanovic via cfe-commits]

nemanjai added inline comments.

================
Comment at: lib/Headers/altivec.h:136
@@ -131,3 +135,3 @@
 
 #if defined(__POWER8_VECTOR__) && defined(__powerpc64__)
 static vector double __ATTRS_o_ai vec_abs(vector double __a) {
----------------
I thought we were going to change the guard here to __VSX__ rather than __POWER8_VECTOR.

================
Comment at: test/CodeGen/builtins-ppc-altivec.c:3
@@ +2,3 @@
+// RUN: %clang_cc1 -faltivec -triple powerpc-unknown-unknown -emit-llvm %s \
+// RUN: -o - | FileCheck %s
+// RUN: %clang_cc1 -faltivec -triple powerpc64-unknown-unknown -emit-llvm %s \
----------------
Just a minor nit. Please indent the continuation lines for readability.

================
Comment at: test/CodeGen/builtins-ppc-altivec.c:8
@@ +7,3 @@
+// RUN:  -o - | FileCheck %s -check-prefix=CHECK-LE
+// RUN: not %clang_cc1 -triple powerpc64le-unknown-unknown -emit-llvm %s \
+// RUN: -o - 2>&1 | FileCheck %s -check-prefix=CHECK-NOALTIVEC
----------------
I would have thought that we already have a diagnostic test case for the missing -faltivec, but if we don't, thanks for adding it.

================
Comment at: test/CodeGen/builtins-ppc-altivec.c:84
@@ +83,3 @@
+// CHECK-LE: store <4 x float> %{{.*}}, <4 x float>* @vf
+// CHECK-NOALTIVEC: error: use of undeclared identifier 'vf'
+// CHECK-NOALTIVEC: vf = vec_abs(vf) 
----------------
I am not recommending you change anything here, just want to point out that there's a potential concern with adding diagnostic test cases in with functional ones (and yes, I know we have lots of this already). Namely, the concern is that the diagnostic may go away as we add more code to the test case and the particular error you're looking for goes past the error threshold (I think the default is 20 errors).

================
Comment at: test/CodeGen/builtins-ppc-p8vector.c:76
@@ -75,5 +75,3 @@
   res_vd = vec_abs(vda);
-// CHECK: store <2 x i64> <i64 9223372036854775807, i64 9223372036854775807>, <2 x i64>*
-// CHECK: and <2 x i64>
-// CHECK-LE: store <2 x i64> <i64 9223372036854775807, i64 9223372036854775807>, <2 x i64>*
-// CHECK-LE: and <2 x i64>
+// CHECK: call <2 x double> @llvm.fabs.v2f64(<2 x double> %{{[0-9]*}})
+// CHECK: store <2 x double> %{{.*}}, <2 x double>* @res_vd
----------------
I think that without asserts, clang sometimes gives temporary variables names rather than numbers. I'd recommend getting rid of the regex for the argument to @llvm.fabs.* so that you don't get into a weird situation where some build bot somewhere fails due to this change.


http://reviews.llvm.org/D17816