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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
