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

[Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>]

> On Aug 1, 2017, at 1:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> 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 :(. 

Jakub, thanks for handling!  I'll put the cleanup of this diagnostic
message on our list.  Thanks for pointing it out.

Bill

> 
> 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
>