[PATCH, rs6000] altivec_resolve_overloaded_builtin fixes (PR target/81622)

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

On Mon, Jul 31, 2017 at 02:42:21PM -0500, Bill Schmidt wrote:
> > On Jul 31, 2017, at 11:27 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Jul 31, 2017 at 11:19:26AM -0500, Bill Schmidt wrote:
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81622 reports an ICE-on-invalid
> >> for the vec_ld and vec_st built-in functions.  This fires when the last
> >> argument of the built-in is not a pointer or array type, as is required.
> >> We break on this during early expansion of the built-ins into tree code
> >> during parsing.  The solution, as with other ill-formed uses of these
> >> built-ins, is to avoid the early expansion when the argument has an invalid
> >> type, so that normal error handling can kick in later.
> >> 
> >> (The long-term solution is to move the vec_ld and vec_st built-ins to the
> >> gimple folding work that Will Schmidt has been doing, but that hasn't
> >> happened yet.)
> >> 
> >> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
> >> Is this ok for trunk and GCC 7?  I'd like to get it into 7.2 since it
> >> is a 7 regression.
> > 
> > See the patch I've attached in the PR, this isn't sufficient
> > (and for the ARRAY_TYPE I wonder if you can ever see there ARRAY_TYPE),
> > the function has various other issues, including e.g. ICE on
> > vec_cmpne (1, 2) with -mpower9.
> 
> Yes, I'll withdraw the patch (but the ARRAY_TYPE thing is necessary as
> we've discussed).  I'll step out of your way on this one since you've got it
> well in hand.  It would be great to have a fix in for 7.2, though.

Here is the variant patch.  In addition to fixing the ICE for vec_ld, for
vec_st it just moves the premature computation of aligned to the point where
it is used and that is after we've also verified that the types of the call
arguments match the builtin argument types, it fixes ICE on vec_cmpne, where
for -mpower9 it accesses TYPE_MODE (TREE_TYPE (arg0_type)) without checking
that arg0_type is actually VECTOR_TYPE (if it is e.g. INTEGRAL_TYPE, it will
segfault) and has some formatting fixes too.

Bootstrapped/regtested on powerpc64{,le}-linux, ok for
trunk/7.2 (for the latter it applies without the 2 formatting fix hunks
with while (desc->code && desc->code == fcode &&)?

Note, there is diagnostic message in that routine starting with "Builtin,
that is something that should be fixed too, GCC diagnostic messages
(except for Fortran) don't start with a capital letter.  But this change
requires to adjust quite a lot of testcases in gcc.target/powerpc :(. 

2017-07-31  Jakub Jelinek  <jakub@redhat.com>

	PR target/81622
	* config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin): For
	__builtin_vec_cmpne verify both arguments are compatible vectors
	before looking at TYPE_MODE on the element type.  For __builtin_vec_ld
	verify arg1_type is a pointer or array type.  For __builtin_vec_st,
	move computation of aligned to after checking the argument types.
	Formatting fixes.

	* gcc.target/powerpc/pr81622.c: New test.

--- gcc/config/rs6000/rs6000-c.c.jj	2017-07-25 12:19:51.000000000 +0200
+++ gcc/config/rs6000/rs6000-c.c	2017-07-31 17:05:59.947783972 +0200
@@ -5852,6 +5852,12 @@ altivec_resolve_overloaded_builtin (loca
       tree arg1 = (*arglist)[1];
       tree arg1_type = TREE_TYPE (arg1);
 
+      /* Both arguments must be vectors and the types must be compatible.  */
+      if (TREE_CODE (arg0_type) != VECTOR_TYPE)
+	goto bad;
+      if (!lang_hooks.types_compatible_p (arg0_type, arg1_type))
+	goto bad;
+
       /* Power9 instructions provide the most efficient implementation of
 	 ALTIVEC_BUILTIN_VEC_CMPNE if the mode is not DImode or TImode
 	 or SFmode or DFmode.  */
@@ -5861,12 +5867,6 @@ altivec_resolve_overloaded_builtin (loca
 	  || (TYPE_MODE (TREE_TYPE (arg0_type)) == SFmode)
 	  || (TYPE_MODE (TREE_TYPE (arg0_type)) == DFmode))
 	{
-	  /* Both arguments must be vectors and the types must be compatible.  */
-	  if (TREE_CODE (arg0_type) != VECTOR_TYPE)
-	    goto bad;
-	  if (!lang_hooks.types_compatible_p (arg0_type, arg1_type))
-	    goto bad;
-
 	  switch (TYPE_MODE (TREE_TYPE (arg0_type)))
 	    {
 	      /* vec_cmpneq (va, vb) == vec_nor (vec_cmpeq (va, vb),
@@ -5931,8 +5931,8 @@ altivec_resolve_overloaded_builtin (loca
 	 __int128) and the types must be compatible.  */
       if (TREE_CODE (arg0_type) != VECTOR_TYPE)
 	goto bad;
-      if (!lang_hooks.types_compatible_p (arg0_type, arg1_type) ||
-	  !lang_hooks.types_compatible_p (arg1_type, arg2_type))
+      if (!lang_hooks.types_compatible_p (arg0_type, arg1_type)
+	  || !lang_hooks.types_compatible_p (arg1_type, arg2_type))
 	goto bad;
 
       switch (TYPE_MODE (TREE_TYPE (arg0_type)))
@@ -6014,8 +6014,8 @@ altivec_resolve_overloaded_builtin (loca
 	 __int128) and the types must be compatible.  */
       if (TREE_CODE (arg0_type) != VECTOR_TYPE)
 	goto bad;
-      if (!lang_hooks.types_compatible_p (arg0_type, arg1_type) ||
-	  !lang_hooks.types_compatible_p (arg1_type, arg2_type))
+      if (!lang_hooks.types_compatible_p (arg0_type, arg1_type)
+	  || !lang_hooks.types_compatible_p (arg1_type, arg2_type))
 	goto bad;
 
       switch (TYPE_MODE (TREE_TYPE (arg0_type)))
@@ -6464,6 +6464,9 @@ altivec_resolve_overloaded_builtin (loca
 
       /* Strip qualifiers like "const" from the pointer arg.  */
       tree arg1_type = TREE_TYPE (arg1);
+      if (!POINTER_TYPE_P (arg1_type) && TREE_CODE (arg1_type) != ARRAY_TYPE)
+	goto bad;
+
       tree inner_type = TREE_TYPE (arg1_type);
       if (TYPE_QUALS (TREE_TYPE (arg1_type)) != 0)
 	{
@@ -6552,11 +6555,6 @@ altivec_resolve_overloaded_builtin (loca
 	      arg2 = build1 (ADDR_EXPR, arg2_type, arg2_elt0);
 	    }
 
-	  tree addr = fold_build2_loc (loc, POINTER_PLUS_EXPR, arg2_type,
-				       arg2, arg1);
-	  tree aligned = fold_build2_loc (loc, BIT_AND_EXPR, arg2_type, addr,
-					  build_int_cst (arg2_type, -16));
-
 	  /* Find the built-in to make sure a compatible one exists; if not
 	     we fall back to default handling to get the error message.  */
 	  for (desc = altivec_overloaded_builtins;
@@ -6569,6 +6567,12 @@ altivec_resolve_overloaded_builtin (loca
 		&& rs6000_builtin_type_compatible (TREE_TYPE (arg2),
 						   desc->op3))
 	      {
+		tree addr = fold_build2_loc (loc, POINTER_PLUS_EXPR, arg2_type,
+					     arg2, arg1);
+		tree aligned
+		  = fold_build2_loc (loc, BIT_AND_EXPR, arg2_type,
+				     addr, build_int_cst (arg2_type, -16));
+
 		tree arg0_type = TREE_TYPE (arg0);
 		if (TYPE_MODE (arg0_type) == V2DImode)
 		  /* Type-based aliasing analysis thinks vector long
@@ -6694,8 +6698,8 @@ altivec_resolve_overloaded_builtin (loca
 	    overloaded_code = P6_BUILTIN_CMPB_32;
 	  }
 
-	while (desc->code && desc->code == fcode &&
-	       desc->overloaded_code != overloaded_code)
+	while (desc->code && desc->code == fcode
+	       && desc->overloaded_code != overloaded_code)
 	  desc++;
 
 	if (desc->code && (desc->code == fcode)
@@ -6741,8 +6745,8 @@ altivec_resolve_overloaded_builtin (loca
 	    else
 	      overloaded_code = P9V_BUILTIN_VSIEDP;
 	  }
-	while (desc->code && desc->code == fcode &&
-	       desc->overloaded_code != overloaded_code)
+	while (desc->code && desc->code == fcode
+	       && desc->overloaded_code != overloaded_code)
 	  desc++;
 	if (desc->code && (desc->code == fcode)
 	    && rs6000_builtin_type_compatible (types[0], desc->op1)
@@ -6784,9 +6788,9 @@ altivec_resolve_overloaded_builtin (loca
       }
   }
  bad:
-    {
-      const char *name = rs6000_overloaded_builtin_name (fcode);
-      error ("invalid parameter combination for AltiVec intrinsic %s", name);
-      return error_mark_node;
-    }
+  {
+    const char *name = rs6000_overloaded_builtin_name (fcode);
+    error ("invalid parameter combination for AltiVec intrinsic %s", name);
+    return error_mark_node;
+  }
 }
--- gcc/testsuite/gcc.target/powerpc/pr81622.c.jj	2017-07-31 17:16:26.070493869 +0200
+++ gcc/testsuite/gcc.target/powerpc/pr81622.c	2017-07-31 17:16:18.000000000 +0200
@@ -0,0 +1,13 @@
+/* PR target/81622 */
+/* { dg-do compile { target { powerpc*-*-linux* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power9" } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mcpu=power9 -O2" } */
+
+void
+foo (void)
+{
+  __builtin_vec_ld (1, 2);	/* { dg-error "invalid parameter combination" } */
+  __builtin_vec_cmpne (1, 2);	/* { dg-error "invalid parameter combination" } */
+  __builtin_vec_st (1, 0, 5);	/* { dg-error "invalid parameter combination" } */
+}


	Jakub