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

[Nemanja Ivanovic via cfe-commits]

nemanjai added a comment.

All of my comments are just nits and shouldn't hold up approval. As far as I can tell this looks fine, but I'll let the LGTM come from Kit or Hal.


================
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) {
----------------
amehsan wrote:
> 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.
That would be great! Thank you.

================
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) 
----------------
amehsan wrote:
> 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?
I think that's a great idea and good general guidance for test cases of this type. BTW, no need to post another review for this - just change it in the commit once this patch is approved.

================
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
----------------
amehsan wrote:
> 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 :)
I see what you mean, it's in the previous CHECK pattern. You should be safe keeping this in. However, I'd recommend that you try a build without asserts (especially for FE changes) just to make sure things don't behave differently. The only reason I bring it up is because I've been burned by that before :).


http://reviews.llvm.org/D17816