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

[amehsan via cfe-commits]

amehsan 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) {
----------------
nemanjai wrote:
> I thought we were going to change the guard here to __VSX__ rather than __POWER8_VECTOR.
I want to go through the file separately, check for similar inaccuracies, and fix everything of that kind in one separate commit. This may also require moving test cases between different files. So I do not want to mix it with another submission.

================
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
----------------
nemanjai wrote:
> 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.
No we don't. I spent some time looking for it in the existing test cases and did not find anything.

================
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) 
----------------
nemanjai wrote:
> 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).
Would it address your concern if I add "-ferror-limit 0" to the command line?

================
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
----------------
nemanjai wrote:
> 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.
That pattern is used in other tests. (You can see that in the one right above this one.) But I am fine with changing it to a more general regex like {{.*}}. (I can also remove it but I really wanted to close the parenthesis :)


http://reviews.llvm.org/D17816