diff mbox

PATCH [x86_64] PR20020 - 128 bit structs not targeted to TImode

Message ID 20120814042032.GB23649@intrepid.com
State New
Headers show

Commit Message

Gary Funck Aug. 14, 2012, 4:20 a.m. UTC 
Attached, is a patch to fix PR20020, and three test cases.
This patch improves the code generated for structs that
can be represented in a TImode value on an x86_64 target.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20020

The test cases scan the generated RTL and verify
that TImode operations are present.

Note that this patch will uncover a new test
regression, described here:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20020#c9

Comments

Jakub Jelinek Aug. 14, 2012, 6:30 a.m. UTC | #1 
On Mon, Aug 13, 2012 at 09:20:32PM -0700, Gary Funck wrote:
> --- gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> @@ -0,0 +1,25 @@
> +/* Target is restricted to x86_64 type architectures,
> +   to check that 128-bit struct's are represented
> +   as TImode values.  */
> +/* { dg-require-effective-target int128 } */
> +/* { dg-do compile { target { x86_64-*-* } } } */

Given this all the testcases should go into gcc/testsuite/gcc.target/i386/

	Jakub
Gary Funck Aug. 14, 2012, 11:51 a.m. UTC | #2 
On 08/14/12 08:30:59, Jakub Jelinek wrote:
> On Mon, Aug 13, 2012 at 09:20:32PM -0700, Gary Funck wrote:
> > --- gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> > +++ gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> > @@ -0,0 +1,25 @@
> > +/* Target is restricted to x86_64 type architectures,
> > +   to check that 128-bit struct's are represented
> > +   as TImode values.  */
> > +/* { dg-require-effective-target int128 } */
> > +/* { dg-do compile { target { x86_64-*-* } } } */
> 
> Given this all the testcases should go into gcc/testsuite/gcc.target/i386/

OK.

Note: It might be possible to leave only dg-require-effective-target int128
and use this as a regression test for other targets, such as
PPC64, S390, and IA64.  However, I was uncertain if the RTL would
be similar enough, and know that in at least one case the
RTL scan would have to be adjusted.  Also, I don't have access
to a S390.  If there is interest in generalizing the test,
let me know.  Otherwise, I'll move the tests to gcc.target/i386.

- Gary
Joseph Myers Aug. 14, 2012, 3:33 p.m. UTC | #3 
On Tue, 14 Aug 2012, Jakub Jelinek wrote:

> On Mon, Aug 13, 2012 at 09:20:32PM -0700, Gary Funck wrote:
> > --- gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> > +++ gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> > @@ -0,0 +1,25 @@
> > +/* Target is restricted to x86_64 type architectures,
> > +   to check that 128-bit struct's are represented
> > +   as TImode values.  */
> > +/* { dg-require-effective-target int128 } */
> > +/* { dg-do compile { target { x86_64-*-* } } } */
> 
> Given this all the testcases should go into gcc/testsuite/gcc.target/i386/

And restricting the target to x86_64-*-* is wrong anyway, since any such 
test should also be run for i?86-*-* -m64.  Use { target { ! { ia32 } } } 
instead if you want to disable just -m32 testing, { target lp64 } if you 
only want -m64 testing but not -m32 or -mx32.
Gary Funck Aug. 14, 2012, 4:12 p.m. UTC | #4 
On 08/14/12 15:33:10, Joseph S. Myers wrote:
> On Tue, 14 Aug 2012, Jakub Jelinek wrote:
> 
> > On Mon, Aug 13, 2012 at 09:20:32PM -0700, Gary Funck wrote:
> > > --- gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> > > +++ gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
> > > @@ -0,0 +1,25 @@
> > > +/* Target is restricted to x86_64 type architectures,
> > > +   to check that 128-bit struct's are represented
> > > +   as TImode values.  */
> > > +/* { dg-require-effective-target int128 } */
> > > +/* { dg-do compile { target { x86_64-*-* } } } */
> > 
> > Given this all the testcases should go into gcc/testsuite/gcc.target/i386/
> 
> And restricting the target to x86_64-*-* is wrong anyway, since any such 
> test should also be run for i?86-*-* -m64.  Use { target { ! { ia32 } } } 
> instead if you want to disable just -m32 testing, { target lp64 } if you 
> only want -m64 testing but not -m32 or -mx32.

How about:
1. Move the test to gcc/testsuite/gcc.target/i386/
2. The comment is amended to read:
   /* Check that 128-bit struct's are represented as TImode values.  */
3. This test is retained:
   /* { dg-require-effective-target int128 } */
4. This target test is removed:
   /* { dg-do compile } */

It is possible that "dg-require-effective-target int128" is
too restrictive (in the sense that some x86 target might in
theory support TImode, but not __int128_t), but at least
some reasonable test coverage is guaranteed.

- Gary
H.J. Lu Aug. 14, 2012, 4:33 p.m. UTC | #5 
On Tue, Aug 14, 2012 at 9:12 AM, Gary Funck <gary@intrepid.com> wrote:
> On 08/14/12 15:33:10, Joseph S. Myers wrote:
>> On Tue, 14 Aug 2012, Jakub Jelinek wrote:
>>
>> > On Mon, Aug 13, 2012 at 09:20:32PM -0700, Gary Funck wrote:
>> > > --- gcc/testsuite/gcc.dg/pr20020-1.c      (revision 0)
>> > > +++ gcc/testsuite/gcc.dg/pr20020-1.c      (revision 0)
>> > > @@ -0,0 +1,25 @@
>> > > +/* Target is restricted to x86_64 type architectures,
>> > > +   to check that 128-bit struct's are represented
>> > > +   as TImode values.  */
>> > > +/* { dg-require-effective-target int128 } */
>> > > +/* { dg-do compile { target { x86_64-*-* } } } */
>> >
>> > Given this all the testcases should go into gcc/testsuite/gcc.target/i386/
>>
>> And restricting the target to x86_64-*-* is wrong anyway, since any such
>> test should also be run for i?86-*-* -m64.  Use { target { ! { ia32 } } }
>> instead if you want to disable just -m32 testing, { target lp64 } if you
>> only want -m64 testing but not -m32 or -mx32.
>
> How about:
> 1. Move the test to gcc/testsuite/gcc.target/i386/
> 2. The comment is amended to read:
>    /* Check that 128-bit struct's are represented as TImode values.  */
> 3. This test is retained:
>    /* { dg-require-effective-target int128 } */
> 4. This target test is removed:
>    /* { dg-do compile } */
>
> It is possible that "dg-require-effective-target int128" is
> too restrictive (in the sense that some x86 target might in
> theory support TImode, but not __int128_t), but at least
> some reasonable test coverage is guaranteed.

I believe int128 requirement is correct.
diff mbox

Patch

Index: gcc/config/i386/i386.h
===================================================================
--- gcc/config/i386/i386.h	(revision 190336)
+++ gcc/config/i386/i386.h	(working copy)
@@ -1820,6 +1820,10 @@  do {							\
 #define BRANCH_COST(speed_p, predictable_p) \
   (!(speed_p) ? 2 : (predictable_p) ? 0 : ix86_branch_cost)
 
+/* An integer expression for the size in bits of the largest integer machine
+   mode that should actually be used.  We allow pairs of registers.  */
+#define MAX_FIXED_MODE_SIZE GET_MODE_BITSIZE (TARGET_64BIT ? TImode : DImode)
+
 /* Define this macro as a C expression which is nonzero if accessing
    less than a word of memory (i.e. a `char' or a `short') is no
    faster than accessing a word of memory, i.e., if such access
Index: gcc/testsuite/gcc.dg/pr20020-1.c
===================================================================
--- gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr20020-1.c	(revision 0)
@@ -0,0 +1,25 @@ 
+/* Target is restricted to x86_64 type architectures,
+   to check that 128-bit struct's are represented
+   as TImode values.  */
+/* { dg-require-effective-target int128 } */
+/* { dg-do compile { target { x86_64-*-* } } } */
+/* { dg-options "-O2 -fdump-rtl-expand" } */
+
+struct shared_ptr_struct
+{
+  unsigned long long phase:48;
+  unsigned short thread:16;
+  void *addr;
+};
+typedef struct shared_ptr_struct sptr_t;
+
+sptr_t S;
+
+sptr_t
+sptr_result (void)
+{
+  return S;
+}
+/* { dg-final { scan-rtl-dump "\\\(set \\\(reg:TI \[0-9\]* \\\[ <retval> \\\]\\\)" "expand" } } */
+/* { dg-final { scan-rtl-dump "\\\(set \\\(reg/i:TI 0 ax\\\)" "expand" } } */
+/* { dg-final { cleanup-rtl-dump "expand" } } */
Index: gcc/testsuite/gcc.dg/pr20020-2.c
===================================================================
--- gcc/testsuite/gcc.dg/pr20020-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr20020-2.c	(revision 0)
@@ -0,0 +1,23 @@ 
+/* Target is restricted to x86_64 type architectures,
+   to check that 128-bit struct's are represented
+   as TImode values.  */
+/* { dg-require-effective-target int128 } */
+/* { dg-do compile { target { x86_64-*-* } } } */
+/* { dg-options "-O2 -fdump-rtl-expand" } */
+
+struct shared_ptr_struct
+{
+  unsigned long long phase:48;
+  unsigned short thread:16;
+  void *addr;
+};
+typedef struct shared_ptr_struct sptr_t;
+
+void
+copy_sptr (sptr_t *dest, sptr_t src)
+{
+  *dest = src;
+}
+
+/* { dg-final { scan-rtl-dump "\\\(set \\\(reg:TI \[0-9\]*" "expand" } } */
+/* { dg-final { cleanup-rtl-dump "expand" } } */
Index: gcc/testsuite/gcc.dg/pr20020-3.c
===================================================================
--- gcc/testsuite/gcc.dg/pr20020-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr20020-3.c	(revision 0)
@@ -0,0 +1,26 @@ 
+/* Target is restricted to x86_64 type architectures,
+   to check that 128-bit struct's are represented
+   as TImode values.  */
+/* { dg-require-effective-target int128 } */
+/* { dg-do compile { target { x86_64-*-* } } } */
+/* { dg-options "-O2 -fdump-rtl-expand" } */
+
+struct shared_ptr_struct
+{
+  unsigned long long phase:48;
+  unsigned short thread:16;
+  void *addr;
+};
+typedef struct shared_ptr_struct sptr_t;
+
+sptr_t sptr_1, sptr_2;
+
+void
+copy_sptr (void)
+{
+  sptr_1 = sptr_2;  
+}
+
+/* { dg-final { scan-rtl-dump "\\\(set \\\(reg:TI \[0-9\]*" "expand" } } */
+/* { dg-final { scan-rtl-dump "\\\(set \\\(mem/c:TI" "expand" } } */
+/* { dg-final { cleanup-rtl-dump "expand" } } */