Message ID | 20170912142548.GI1701@tucnak |
---|---|
State | New |
Headers | show |
Series | Fix powerpc ICE with __builtin_vec_ld on an array (PR target/82112, take 2) | expand |
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 >
--- 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" } + +#include <altivec.h> + +__attribute__((aligned (16))) extern const unsigned char c[16]; + +void +foo (void) +{ + vec_ld (0, c); +}