> On Sep 12, 2017, at 10:00 AM, Segher Boessenkool <seg...@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 <ja...@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
>
