Message ID | 15c6036f67ae9d5c516c9de8506bacc36f1af4f6.camel@vnet.ibm.com |
---|---|
State | New |
Headers | show |
Series | [rs6000] Fix Vector long long subtype (PR96139) | expand |
Hi Will, On Tue, Sep 01, 2020 at 09:00:20PM -0500, will schmidt wrote: > This corrects an issue with the powerpc vector long long subtypes. > As reported by SjMunroe in PR96139. When building some code with -Wall > and attempting to print an element of a "long long vector" with a long long > printf format string, we will report a error because the vector sub-type > was improperly defined as int. > > When defining a V2DI_type_node we use a TARGET_POWERPC64 ternary to > define the V2DI_type_node with "vector long" or "vector long long". > We also need to specify the proper sub-type when we define the type. > - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? "__vector long" > - : "__vector long long", > - intDI_type_node, 2); > + V2DI_type_node > + = rs6000_vector_type (TARGET_POWERPC64 > + ? "__vector long" : "__vector long long", > + TARGET_POWERPC64 > + ? long_long_integer_type_node : intDI_type_node, > + 2); Can't you just use long_long_integer_type_node in all cases? Or, what else is intDI_type_node for 32 bit? > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr96139-a.c > @@ -0,0 +1,32 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -Wall -m32 " } */ (trailing space, here and elsewhere -- not that it matters of course) Segher
On Wed, 2020-09-02 at 05:13 -0500, Segher Boessenkool wrote: > Hi Will, > > On Tue, Sep 01, 2020 at 09:00:20PM -0500, will schmidt wrote: > > This corrects an issue with the powerpc vector long long > > subtypes. > > As reported by SjMunroe in PR96139. When building some code with > > -Wall > > and attempting to print an element of a "long long vector" with a > > long long > > printf format string, we will report a error because the vector > > sub-type > > was improperly defined as int. > > > > When defining a V2DI_type_node we use a TARGET_POWERPC64 ternary to > > define the V2DI_type_node with "vector long" or "vector long long". > > We also need to specify the proper sub-type when we define the > > type. > > - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? > > "__vector long" > > - : "__vector long long", > > - intDI_type_node, 2); > > + V2DI_type_node > > + = rs6000_vector_type (TARGET_POWERPC64 > > + ? "__vector long" : "__vector long long", > > + TARGET_POWERPC64 > > + ? long_long_integer_type_node : > > intDI_type_node, > > + 2); > > Can't you just use long_long_integer_type_node in all cases? Or, > what > else is intDI_type_node for 32 bit? I'm not sure offhand. I'm tending to assume the use of intDI_type_node is critical for some underlying reason. I'll give this a spin with just long_long_integer_type_node and see what happens. thanks, > > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/powerpc/pr96139-a.c > > @@ -0,0 +1,32 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -Wall -m32 " } */ > > (trailing space, here and elsewhere -- not that it matters of course) yup, will fix. thanks for the review. -Will > > > Segher
Hi! On Thu, Sep 03, 2020 at 10:37:33AM -0500, will schmidt wrote: > On Wed, 2020-09-02 at 05:13 -0500, Segher Boessenkool wrote: > > On Tue, Sep 01, 2020 at 09:00:20PM -0500, will schmidt wrote: > > > This corrects an issue with the powerpc vector long long > > > subtypes. > > > As reported by SjMunroe in PR96139. When building some code with > > > -Wall > > > and attempting to print an element of a "long long vector" with a > > > long long > > > printf format string, we will report a error because the vector > > > sub-type > > > was improperly defined as int. > > > > > > When defining a V2DI_type_node we use a TARGET_POWERPC64 ternary to > > > define the V2DI_type_node with "vector long" or "vector long long". > > > We also need to specify the proper sub-type when we define the > > > type. > > > - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? > > > "__vector long" > > > - : "__vector long long", > > > - intDI_type_node, 2); > > > + V2DI_type_node > > > + = rs6000_vector_type (TARGET_POWERPC64 > > > + ? "__vector long" : "__vector long long", > > > + TARGET_POWERPC64 > > > + ? long_long_integer_type_node : > > > intDI_type_node, > > > + 2); > > > > Can't you just use long_long_integer_type_node in all cases? Or, > > what > > else is intDI_type_node for 32 bit? > > I'm not sure offhand. I'm tending to assume the use of intDI_type_node > is critical for some underlying reason. I'll give this a spin with > just long_long_integer_type_node and see what happens. If that works, that is okay for trunk (and all backports you want). If not, just use what you sent. Thanks! Segher
On Thu, Sep 3, 2020 at 8:10 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Thu, Sep 03, 2020 at 10:37:33AM -0500, will schmidt wrote: > > On Wed, 2020-09-02 at 05:13 -0500, Segher Boessenkool wrote: > > > On Tue, Sep 01, 2020 at 09:00:20PM -0500, will schmidt wrote: > > > > This corrects an issue with the powerpc vector long long > > > > subtypes. > > > > As reported by SjMunroe in PR96139. When building some code with > > > > -Wall > > > > and attempting to print an element of a "long long vector" with a > > > > long long > > > > printf format string, we will report a error because the vector > > > > sub-type > > > > was improperly defined as int. > > > > > > > > When defining a V2DI_type_node we use a TARGET_POWERPC64 ternary to > > > > define the V2DI_type_node with "vector long" or "vector long long". > > > > We also need to specify the proper sub-type when we define the > > > > type. > > > > - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? > > > > "__vector long" > > > > - : "__vector long long", > > > > - intDI_type_node, 2); > > > > + V2DI_type_node > > > > + = rs6000_vector_type (TARGET_POWERPC64 > > > > + ? "__vector long" : "__vector long long", > > > > + TARGET_POWERPC64 > > > > + ? long_long_integer_type_node : > > > > intDI_type_node, > > > > + 2); > > > > > > Can't you just use long_long_integer_type_node in all cases? Or, > > > what > > > else is intDI_type_node for 32 bit? > > > > I'm not sure offhand. I'm tending to assume the use of intDI_type_node > > is critical for some underlying reason. I'll give this a spin with > > just long_long_integer_type_node and see what happens. > > If that works, that is okay for trunk (and all backports you want). If > not, just use what you sent. Beware of alias issues! 'long' and 'long long' are distinct types even if they have the same size and long * aliases with vector<long> * but not with vector<long long> *. So if the user writes 'vector long' you should use the appropriate component type. If the user writes 'vector intDI_type' you should use intDI_type. Richard. > Thanks! > > > Segher
Hi! On Fri, Sep 04, 2020 at 08:55:43AM +0200, Richard Biener wrote: > On Thu, Sep 3, 2020 at 8:10 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > On Thu, Sep 03, 2020 at 10:37:33AM -0500, will schmidt wrote: > > > On Wed, 2020-09-02 at 05:13 -0500, Segher Boessenkool wrote: > > > > On Tue, Sep 01, 2020 at 09:00:20PM -0500, will schmidt wrote: > > > > > - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? > > > > > "__vector long" > > > > > - : "__vector long long", > > > > > - intDI_type_node, 2); > > > > > + V2DI_type_node > > > > > + = rs6000_vector_type (TARGET_POWERPC64 > > > > > + ? "__vector long" : "__vector long long", > > > > > + TARGET_POWERPC64 > > > > > + ? long_long_integer_type_node : > > > > > intDI_type_node, > > > > > + 2); > > > > > > > > Can't you just use long_long_integer_type_node in all cases? Or, > > > > what > > > > else is intDI_type_node for 32 bit? > > > > > > I'm not sure offhand. I'm tending to assume the use of intDI_type_node > > > is critical for some underlying reason. I'll give this a spin with > > > just long_long_integer_type_node and see what happens. > > > > If that works, that is okay for trunk (and all backports you want). If > > not, just use what you sent. > > Beware of alias issues! 'long' and 'long long' are distinct types even if > they have the same size and long * aliases with vector<long> * but not > with vector<long long> *. That is what this patch is all about, so I do beware (right now ;-) ), don't worry :-) > So if the user writes 'vector long' you should use the appropriate component > type. If the user writes 'vector intDI_type' you should use intDI_type. Yes. But intDI is not something the user can write, it is an internal name, and for 32 bit it is just long long int. We use intDI if something is long on 64 bit and long long on 32 bit, but here we always want long long, so we can just as well just write that :-) intDI_type_node = make_or_reuse_type (GET_MODE_BITSIZE (DImode), 0); Segher
On Fri, 2020-09-04 at 03:47 -0500, Segher Boessenkool wrote: > Hi! > > On Fri, Sep 04, 2020 at 08:55:43AM +0200, Richard Biener wrote: > > On Thu, Sep 3, 2020 at 8:10 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > On Thu, Sep 03, 2020 at 10:37:33AM -0500, will schmidt wrote: > > > > On Wed, 2020-09-02 at 05:13 -0500, Segher Boessenkool wrote: > > > > > On Tue, Sep 01, 2020 at 09:00:20PM -0500, will schmidt wrote: > > > > > > - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? > > > > > > "__vector long" > > > > > > - : "__vector long long", > > > > > > - intDI_type_node, 2); > > > > > > + V2DI_type_node > > > > > > + = rs6000_vector_type (TARGET_POWERPC64 > > > > > > + ? "__vector long" : "__vector long > > > > > > long", > > > > > > + TARGET_POWERPC64 > > > > > > + ? long_long_integer_type_node : > > > > > > intDI_type_node, > > > > > > + 2); > > > > > > > > > > Can't you just use long_long_integer_type_node in all > > > > > cases? Or, > > > > > what > > > > > else is intDI_type_node for 32 bit? > > > > > > > > I'm not sure offhand. I'm tending to assume the use of > > > > intDI_type_node > > > > is critical for some underlying reason. I'll give this a > > > > spin with > > > > just long_long_integer_type_node and see what happens. > > > > > > If that works, that is okay for trunk (and all backports you > > > want). If > > > not, just use what you sent. > > > > Beware of alias issues! 'long' and 'long long' are distinct types > > even if > > they have the same size and long * aliases with vector<long> * but > > not > > with vector<long long> *. > > That is what this patch is all about, so I do beware (right now ;-) > ), > don't worry :-) > > > So if the user writes 'vector long' you should use the appropriate > > component > > type. If the user writes 'vector intDI_type' you should use > > intDI_type. > > Yes. But intDI is not something the user can write, it is an > internal > name, and for 32 bit it is just long long int. We use intDI if > something is long on 64 bit and long long on 32 bit, but here we > always > want long long, so we can just as well just write that :-) > > intDI_type_node = make_or_reuse_type (GET_MODE_BITSIZE (DImode), > 0); Thanks for the additional scrutiny and discussion.. :-) My regtests with that change passed cleanly, so it was committed yesterday. Thanks, -Will > > > Segher
diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index ff4c2537fada..354cac575cc6 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -12511,13 +12511,16 @@ rs6000_init_builtins (void) if (TARGET_DEBUG_BUILTIN) fprintf (stderr, "rs6000_init_builtins%s%s\n", (TARGET_ALTIVEC) ? ", altivec" : "", (TARGET_VSX) ? ", vsx" : ""); - V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 ? "__vector long" - : "__vector long long", - intDI_type_node, 2); + V2DI_type_node + = rs6000_vector_type (TARGET_POWERPC64 + ? "__vector long" : "__vector long long", + TARGET_POWERPC64 + ? long_long_integer_type_node : intDI_type_node, + 2); V2DF_type_node = rs6000_vector_type ("__vector double", double_type_node, 2); V4SI_type_node = rs6000_vector_type ("__vector signed int", intSI_type_node, 4); V4SF_type_node = rs6000_vector_type ("__vector float", float_type_node, 4); V8HI_type_node = rs6000_vector_type ("__vector signed short", @@ -12529,14 +12532,18 @@ rs6000_init_builtins (void) unsigned_intQI_type_node, 16); unsigned_V8HI_type_node = rs6000_vector_type ("__vector unsigned short", unsigned_intHI_type_node, 8); unsigned_V4SI_type_node = rs6000_vector_type ("__vector unsigned int", unsigned_intSI_type_node, 4); - unsigned_V2DI_type_node = rs6000_vector_type (TARGET_POWERPC64 - ? "__vector unsigned long" - : "__vector unsigned long long", - unsigned_intDI_type_node, 2); + unsigned_V2DI_type_node + = rs6000_vector_type (TARGET_POWERPC64 + ? "__vector unsigned long" + : "__vector unsigned long long", + TARGET_POWERPC64 + ? long_long_unsigned_type_node + : unsigned_intDI_type_node, + 2); opaque_V4SI_type_node = build_opaque_vector_type (intSI_type_node, 4); const_str_type_node = build_pointer_type (build_qualified_type (char_type_node, diff --git a/gcc/testsuite/gcc.target/powerpc/pr96139-a.c b/gcc/testsuite/gcc.target/powerpc/pr96139-a.c new file mode 100644 index 000000000000..37820b1809ba --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr96139-a.c @@ -0,0 +1,32 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wall -m32 " } */ +/* { dg-require-effective-target ilp32 } */ +/* { dg-require-effective-target powerpc_altivec_ok } */ + +#include <stdio.h> +#include <altivec.h> + +void +try_printing_longlong_a ( + __vector signed char cval, + __vector signed int ival, + __vector signed long long int llval, + int x, int y, int z) +{ + printf (" %016llx \n", llval[x]); + printf (" %016x \n", ival[z]); + printf (" %c \n", cval[y]); +} + +void +try_printing_unsigned_longlong_a ( + __vector unsigned char cval, + __vector unsigned int ival, + __vector unsigned long long int llval, + int x, int y, int z) +{ + printf (" %016llx \n", llval[x]); + printf (" %016x \n", ival[z]); + printf (" %c \n", cval[y]); +} + diff --git a/gcc/testsuite/gcc.target/powerpc/pr96139-b.c b/gcc/testsuite/gcc.target/powerpc/pr96139-b.c new file mode 100644 index 000000000000..7fac6901790e --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr96139-b.c @@ -0,0 +1,32 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wall -m64 " } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-require-effective-target powerpc_altivec_ok } */ + +#include <stdio.h> +#include <altivec.h> + +void +try_printing_longlong_a ( + __vector signed char cval, + __vector signed int ival, + __vector signed long long int llval, + int x, int y, int z) +{ + printf (" %016llx \n", llval[x]); + printf (" %016x \n", ival[z]); + printf (" %c \n", cval[y]); +} + + +void +try_printing_unsigned_longlong_a ( + __vector unsigned char cval, + __vector unsigned int ival, + __vector unsigned long long int llval, + int x, int y, int z) +{ + printf (" %016llx \n", llval[x]); + printf (" %016x \n", ival[z]); + printf (" %c \n", cval[y]); +} diff --git a/gcc/testsuite/gcc.target/powerpc/pr96139-c.c b/gcc/testsuite/gcc.target/powerpc/pr96139-c.c new file mode 100644 index 000000000000..2464b8da4f71 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr96139-c.c @@ -0,0 +1,26 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -Wall" } */ +/* { dg-require-effective-target powerpc_altivec_ok } */ + +/* + * Based on test created by sjmunroe for pr96139 + */ + +#include <stdio.h> +#include <altivec.h> + +volatile vector long long llfoo; + +void +print_v2xint64_b () { + printf (" %016llx \n", llfoo[0]); + printf (" %016llx \n", llfoo[1]); +} + +int +main() { +llfoo[0]=12345678; +llfoo[1]=34567890; +print_v2xint64_b(); +return 0; +}