Fix powerpc ICE with __builtin_vec_ld on an array (PR target/82112, take 2)

Hi!

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.
> 
> Bootstrapped/regtested on powerpc64-linux (regtest with {,-m32}), ok for
> trunk?

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?

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.



	Jakub

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

On Tue, Sep 12, 2017 at 10:00:44AM -0500, Segher Boessenkool wrote:
> > --- 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?

It is not really needed, I've just copied/pasted those 3 lines
from some other testcase (which probably used it uselessly as well).
-save-temps can make sense for dg-do link/run testcases where somebody
also wants to dg-final scan-assembler, or for the very rare case when
actually the -save-temps behavior needs to be tested (using separate
preprocessor etc.).  Will fix.

	Jakub

> On Sep 12, 2017, at 10:00 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> 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 :-)

This looks okay to me as well.  Jakub, FYI, Will Schmidt just posted a patch
that will delete all of this code and replace it with early gimple folding, which
will save us from these tedious parsing issues.  But I think we should take
this patch first and consider backports to 6 and 7, since those releases will
still use the parser hooks.

(Will's patch is only for vec_ld, but he will follow up with a vec_st patch
shortly.)

Thanks,
Bill
> 
> 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
>