diff mbox series

[rs6000] Fix Vector long long subtype (PR96139)

Message ID 15c6036f67ae9d5c516c9de8506bacc36f1af4f6.camel@vnet.ibm.com
State New
Headers show
Series [rs6000] Fix Vector long long subtype (PR96139) | expand

Commit Message

will schmidt Sept. 2, 2020, 2 a.m. UTC
Hi,
  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.    

This has been successfully regtested on assorted power7,power8,
power8 targets.  Another sniff test in the queue to verify a last
minute testcase tweak.

OK for trunk?
    
Thanks
-Will
    
    
    PR target/96139
    
    2020-09-01  Will Schmidt  <will_schmidt@vnet.ibm.com>
    
    gcc/Changelog:
      * config/rs6000/rs6000-call.c (rs6000_init_builtin): Update V2DI_type_node
        and unsigned_V2DI_type_node definitions.
    
    gcc/testsuite/Changelog:
      * testsuite/gcc.target/powerpc/pr96139-a.c: New test.
      * testsuite/gcc.target/powerpc/pr96139-b.c: New test.
      * testsuite/gcc.target/powerpc/pr96139-c.c: New test.

Comments

Segher Boessenkool Sept. 2, 2020, 10:13 a.m. UTC | #1
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
will schmidt Sept. 3, 2020, 3:37 p.m. UTC | #2
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
Segher Boessenkool Sept. 3, 2020, 6:09 p.m. UTC | #3
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
Richard Biener Sept. 4, 2020, 6:55 a.m. UTC | #4
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
Segher Boessenkool Sept. 4, 2020, 8:47 a.m. UTC | #5
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
will schmidt Sept. 4, 2020, 2:47 p.m. UTC | #6
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 mbox series

Patch

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;
+}