Re: [PATCH] Fix powerpc ICE with __builtin_vec_ld on an array (PR target/82112, take 2)

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi Jakub,

On Tue, Sep 12, 2017 at 04:25:48PM +0200, Jakub Jelinek wrote:
> On Thu, Sep 07, 2017 at 10:40:30AM +0200, Jakub Jelinek wrote:
> > The C and C++ FE handle resolve_overloaded_builtin differently, the C FE
> > calls it when e.g. array-to-pointer and function-to-pointer conversions
> > are already done on the arguments, while C++ FE does that only much later.
> > The c-common code e.g. for __sync/__atomic builtins deals with
> > that e.g. by:
> >   if (TREE_CODE (type) == ARRAY_TYPE)
> >     {
> >       /* Force array-to-pointer decay for C++.  */
> >       gcc_assert (c_dialect_cxx());
> >       (*params)[0] = default_conversion ((*params)[0]);
> >       type = TREE_TYPE ((*params)[0]);
> >     }
> > while the rs6000 md hook uses default_conversion only in one spot (the
> > generic handling), but for vec_ld and vec_st does something on its own.
> > What is even worse is that for vec_ld, it does that too late, there is
> > a fold_convert in between for the case where the element type is
> > qualified, and that only works if the argument is pointer, not array
> > (in which case it ICEs).
> > So, the following patch moves the vec_ld conversion earlier and for both
> > vec_ld and vec_st uses what c-common as well as later
> > altivec_resolve_overloaded_builtin uses.
> 
> Here is an updated version of that patch, fixed for the C90 non-lvalue
> arrays (which we want to reject as c-family rejects them for __atomic_*
> etc.).  Bootstrapped/regtested on powerpc64-linux (regtest with {,-m32}), ok for
> trunk?

It looks fine to me; Bill, could you take a look?  You know this stuff
much better than I do :-)

One question below:

> 2017-09-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/82112
> 	* config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin): For
> 	ALTIVEC_BUILTIN_VEC_LD if arg1 has array type call default_conversion
> 	on it early, rather than manual conversion late.  For
> 	ALTIVEC_BUILTIN_VEC_ST if arg2 has array type call default_conversion
> 	instead of performing manual conversion.
> 
> 	* gcc.target/powerpc/pr82112.c: New test.
> 	* g++.dg/ext/altivec-18.C: New test.
> 
> --- gcc/config/rs6000/rs6000-c.c.jj	2017-09-08 09:13:59.863591547 +0200
> +++ gcc/config/rs6000/rs6000-c.c	2017-09-08 09:16:36.354738183 +0200
> @@ -6480,7 +6480,13 @@ 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)
> +      if (TREE_CODE (arg1_type) == ARRAY_TYPE && c_dialect_cxx ())
> +	{
> +	  /* Force array-to-pointer decay for C++.  */
> +	  arg1 = default_conversion (arg1);
> +	  arg1_type = TREE_TYPE (arg1);
> +	}
> +      if (!POINTER_TYPE_P (arg1_type))
>  	goto bad;
>  
>        tree inner_type = TREE_TYPE (arg1_type);
> @@ -6500,15 +6506,6 @@ altivec_resolve_overloaded_builtin (loca
>  	  if (!ptrofftype_p (TREE_TYPE (arg0)))
>  	    arg0 = build1 (NOP_EXPR, sizetype, arg0);
>  
> -	  tree arg1_type = TREE_TYPE (arg1);
> -	  if (TREE_CODE (arg1_type) == ARRAY_TYPE)
> -	    {
> -	      arg1_type = TYPE_POINTER_TO (TREE_TYPE (arg1_type));
> -	      tree const0 = build_int_cstu (sizetype, 0);
> -	      tree arg1_elt0 = build_array_ref (loc, arg1, const0);
> -	      arg1 = build1 (ADDR_EXPR, arg1_type, arg1_elt0);
> -	    }
> -
>  	  tree addr = fold_build2_loc (loc, POINTER_PLUS_EXPR, arg1_type,
>  				       arg1, arg0);
>  	  tree aligned = fold_build2_loc (loc, BIT_AND_EXPR, arg1_type, addr,
> @@ -6563,12 +6560,11 @@ altivec_resolve_overloaded_builtin (loca
>  	    arg1 = build1 (NOP_EXPR, sizetype, arg1);
>  
>  	  tree arg2_type = TREE_TYPE (arg2);
> -	  if (TREE_CODE (arg2_type) == ARRAY_TYPE)
> +	  if (TREE_CODE (arg2_type) == ARRAY_TYPE && c_dialect_cxx ())
>  	    {
> -	      arg2_type = TYPE_POINTER_TO (TREE_TYPE (arg2_type));
> -	      tree const0 = build_int_cstu (sizetype, 0);
> -	      tree arg2_elt0 = build_array_ref (loc, arg2, const0);
> -	      arg2 = build1 (ADDR_EXPR, arg2_type, arg2_elt0);
> +	      /* Force array-to-pointer decay for C++.  */
> +	      arg2 = default_conversion (arg2);
> +	      arg2_type = TREE_TYPE (arg2);
>  	    }
>  
>  	  /* Find the built-in to make sure a compatible one exists; if not
> --- gcc/testsuite/gcc.target/powerpc/pr82112.c.jj	2017-09-08 09:20:35.187912843 +0200
> +++ gcc/testsuite/gcc.target/powerpc/pr82112.c	2017-09-08 09:24:21.362376676 +0200
> @@ -0,0 +1,16 @@
> +/* PR target/82112 */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-maltivec -std=gnu90" } */
> +
> +#include <altivec.h>
> +
> +struct __attribute__((aligned (16))) S { unsigned char c[64]; } bar (void);
> +vector unsigned char v;
> +
> +void
> +foo (void)
> +{
> +  vec_ld (0, bar ().c);	/* { dg-error "invalid parameter combination for AltiVec intrinsic" } */
> +  vec_st (v, 0, bar ().c);	/* { dg-error "invalid parameter combination for AltiVec intrinsic" } */
> +}
> --- gcc/testsuite/g++.dg/ext/altivec-18.C.jj	2017-09-08 09:15:20.593774717 +0200
> +++ gcc/testsuite/g++.dg/ext/altivec-18.C	2017-09-08 09:15:20.593774717 +0200
> @@ -0,0 +1,14 @@
> +// PR target/82112
> +// { dg-do compile { target powerpc*-*-* } }
> +// { dg-require-effective-target powerpc_altivec_ok }
> +// { dg-options "-save-temps -maltivec" }

What is this -save-temps for?  Just a leftover?

> +#include <altivec.h>
> +
> +__attribute__((aligned (16))) extern const unsigned char c[16];
> +
> +void
> +foo (void)
> +{
> +  vec_ld (0, c);
> +}

So, okay for trunk as far as I can see.  Thanks,


Segher