[PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[Nathan Froyd]

The patch below fixes a mismatch between what rs6000_legitimize_address
generates and what rs6000_legitimate_address_p expects.  REG+OFFSET
addressing on ppc64 requires that OFFSET be word-aligned;
rs6000_legitimate_address_p checks for that, rs6000_legitimize_address
did not.

The fix is straightforward enough; I think the check for DDmode is
right, although I don't have a hard decimal-float machine to test on.
Testing in progress on powerpc64-unknown-linux-gnu.  OK to commit during
stage 3, or should this wait for 4.5?

-Nathan

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 141763)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2008-11-10  Nathan Froyd  <froydnj@codesourcery.com>
+
+	* config/rs6000/rs6000.c (rs6000_legitimize_address): Check for
+	non-word-aligned REG+CONST addressing.
+
 2008-11-10  Catherine Moore  <clm@codesourcery.com>
 
 	* config.gcc (mips64vrel-*-elf*): Include the tm_file
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 141763)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2008-11-10  Nathan Froyd  <froydnj@codesourcery.com>
+
+	* gcc.target/powerpc/20081110-1.c: New test.
+
 2008-11-10  Catherine Moore  <clm@codesourcery.com>
 
 	* gcc.target/mips/no-smartmips-lwxs.c: New test.
Index: gcc/testsuite/gcc.target/powerpc/20081110-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/20081110-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/20081110-1.c	(revision 0)
@@ -0,0 +1,28 @@
+/* { dg-do compile { target powerpc*-*-* } } */
+/* { dg-options "-O1" } */
+
+/* Verify that we don't ICE by forming invalid addresses for unaligned
+   doubleword loads.  */
+
+struct a
+{
+ unsigned int x;
+ unsigned short y;
+} __attribute__((packed));
+
+struct b {
+ struct a rep;
+ unsigned long long seq;
+} __attribute__((packed));
+
+struct c {
+ int x;
+ struct a a[5460];
+ struct b b;
+};
+
+extern void use_ull(unsigned long long);
+extern void f(struct c *i) {
+  use_ull(i->b.seq);
+  return;
+}
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 141763)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -3816,7 +3816,14 @@ rs6000_legitimize_address (rtx x, rtx ol
       high_int = INTVAL (XEXP (x, 1)) - low_int;
       sum = force_operand (gen_rtx_PLUS (Pmode, XEXP (x, 0),
 					 GEN_INT (high_int)), 0);
-      return gen_rtx_PLUS (Pmode, sum, GEN_INT (low_int));
+      /* Using a REG+CONST 64-bit integer load on 64-bit platforms
+	 requires that CONST be word-aligned.  */
+      if (TARGET_POWERPC64
+	  && (mode == DImode || mode == DDmode)
+	  && (low_int & 0x3))
+	return gen_rtx_PLUS (Pmode, sum, force_reg (Pmode, GEN_INT (low_int)));
+      else
+	return gen_rtx_PLUS (Pmode, sum, GEN_INT (low_int));
     }
   else if (GET_CODE (x) == PLUS
 	   && GET_CODE (XEXP (x, 0)) == REG

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[Andrew Thomas Pinski]

Sent from my iPhone

On Nov 10, 2008, at 8:14 PM, Nathan Froyd <froydnj@codesourcery.com>  
wrote:

> The patch below fixes a mismatch between what  
> rs6000_legitimize_address
> generates and what rs6000_legitimate_address_p expects.  REG+OFFSET
> addressing on ppc64 requires that OFFSET be word-aligned;
> rs6000_legitimate_address_p checks for that, rs6000_legitimize_address
> did not.
>
> The fix is straightforward enough; I think the check for DDmode is
> right, although I don't have a hard decimal-float machine to test on.
> Testing in progress on powerpc64-unknown-linux-gnu.  OK to commit  
> during
> stage 3, or should this wait for 4.5?
>
> -Nathan
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog    (revision 141763)
> +++ gcc/ChangeLog    (working copy)
> @@ -1,3 +1,8 @@
> +2008-11-10  Nathan Froyd  <froydnj@codesourcery.com>
> +
> +    * config/rs6000/rs6000.c (rs6000_legitimize_address): Check for
> +    non-word-aligned REG+CONST addressing.
> +
> 2008-11-10  Catherine Moore  <clm@codesourcery.com>
>
>    * config.gcc (mips64vrel-*-elf*): Include the tm_file
> Index: gcc/testsuite/ChangeLog
> ===================================================================
> --- gcc/testsuite/ChangeLog    (revision 141763)
> +++ gcc/testsuite/ChangeLog    (working copy)
> @@ -1,3 +1,7 @@
> +2008-11-10  Nathan Froyd  <froydnj@codesourcery.com>
> +
> +    * gcc.target/powerpc/20081110-1.c: New test.

Is there a reason why you put this testcase in the target specific  
part of the testsuite rather than the torture tests?  There does not  
seem like anything target specific in it.

Also please send the changelog as plain text and not part of the patch.

Thanks,
Andrew Pinski

>
> +
> 2008-11-10  Catherine Moore  <clm@codesourcery.com>
>
>    * gcc.target/mips/no-smartmips-lwxs.c: New test.
> Index: gcc/testsuite/gcc.target/powerpc/20081110-1.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/20081110-1.c    (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/20081110-1.c    (revision 0)
> @@ -0,0 +1,28 @@
> +/* { dg-do compile { target powerpc*-*-* } } */
> +/* { dg-options "-O1" } */
> +
> +/* Verify that we don't ICE by forming invalid addresses for  
> unaligned
> +   doubleword loads.  */
> +
> +struct a
> +{
> + unsigned int x;
> + unsigned short y;
> +} __attribute__((packed));
> +
> +struct b {
> + struct a rep;
> + unsigned long long seq;
> +} __attribute__((packed));
> +
> +struct c {
> + int x;
> + struct a a[5460];
> + struct b b;
> +};
> +
> +extern void use_ull(unsigned long long);
> +extern void f(struct c *i) {
> +  use_ull(i->b.seq);
> +  return;
> +}
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c    (revision 141763)
> +++ gcc/config/rs6000/rs6000.c    (working copy)
> @@ -3816,7 +3816,14 @@ rs6000_legitimize_address (rtx x, rtx ol
>       high_int = INTVAL (XEXP (x, 1)) - low_int;
>       sum = force_operand (gen_rtx_PLUS (Pmode, XEXP (x, 0),
>                     GEN_INT (high_int)), 0);
> -      return gen_rtx_PLUS (Pmode, sum, GEN_INT (low_int));
> +      /* Using a REG+CONST 64-bit integer load on 64-bit platforms
> +     requires that CONST be word-aligned.  */
> +      if (TARGET_POWERPC64
> +      && (mode == DImode || mode == DDmode)
> +      && (low_int & 0x3))
> +    return gen_rtx_PLUS (Pmode, sum, force_reg (Pmode, GEN_INT  
> (low_int)));
> +      else
> +    return gen_rtx_PLUS (Pmode, sum, GEN_INT (low_int));
>     }
>   else if (GET_CODE (x) == PLUS
>       && GET_CODE (XEXP (x, 0)) == REG

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[David Edelsohn]

On Mon, Nov 10, 2008 at 11:14 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> The patch below fixes a mismatch between what rs6000_legitimize_address
> generates and what rs6000_legitimate_address_p expects.  REG+OFFSET
> addressing on ppc64 requires that OFFSET be word-aligned;
> rs6000_legitimate_address_p checks for that, rs6000_legitimize_address
> did not.
>
> The fix is straightforward enough; I think the check for DDmode is
> right, although I don't have a hard decimal-float machine to test on.
> Testing in progress on powerpc64-unknown-linux-gnu.  OK to commit during
> stage 3, or should this wait for 4.5?

> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 141763)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -3816,7 +3816,14 @@ rs6000_legitimize_address (rtx x, rtx ol
>       high_int = INTVAL (XEXP (x, 1)) - low_int;
>       sum = force_operand (gen_rtx_PLUS (Pmode, XEXP (x, 0),
>                                         GEN_INT (high_int)), 0);
> -      return gen_rtx_PLUS (Pmode, sum, GEN_INT (low_int));
> +      /* Using a REG+CONST 64-bit integer load on 64-bit platforms
> +        requires that CONST be word-aligned.  */
> +      if (TARGET_POWERPC64
> +         && (mode == DImode || mode == DDmode)
> +         && (low_int & 0x3))
> +       return gen_rtx_PLUS (Pmode, sum, force_reg (Pmode, GEN_INT (low_int)));
> +      else
> +       return gen_rtx_PLUS (Pmode, sum, GEN_INT (low_int));
>     }
>   else if (GET_CODE (x) == PLUS
>           && GET_CODE (XEXP (x, 0)) == REG
>

I have no objection to this type of patch during stage 3.

Why create a [REG+REG] indexed form for these unaligned addresses
instead of including the low-order bits in the sum created immediately
prior?  I think the logic can be hoisted to adjust low_int and high_int for
PowerPC64.

Also, as Pinski mentioned, the testcase is not PowerPC-specific.

Thanks, David

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[Nathan Froyd]

On Tue, Nov 11, 2008 at 08:50:15AM -0500, David Edelsohn wrote:
> I have no objection to this type of patch during stage 3.
> 
> Why create a [REG+REG] indexed form for these unaligned addresses
> instead of including the low-order bits in the sum created immediately
> prior?  I think the logic can be hoisted to adjust low_int and high_int for
> PowerPC64.
> 
> Also, as Pinski mentioned, the testcase is not PowerPC-specific.

Changed thusly.  OK?

-Nathan

gcc/
	* config/rs6000/rs6000.c (rs6000_legitimize_address): Check for
	non-word-aligned REG+CONST addressing.

gcc/testsuite/
	* gcc.c-torture/compile/20081111-1.c: New test.

Index: gcc/ChangeLog
===================================================================
Index: gcc/testsuite/gcc.c-torture/compile/20081111-1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/compile/20081111-1.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/compile/20081111-1.c	(revision 0)
@@ -0,0 +1,22 @@
+struct a
+{
+ unsigned int x;
+ unsigned short y;
+} __attribute__((packed));
+
+struct b {
+ struct a rep;
+ unsigned long long seq;
+} __attribute__((packed));
+
+struct c {
+ int x;
+ struct a a[5460];
+ struct b b;
+};
+
+extern void use_ull(unsigned long long);
+extern void f(struct c *i) {
+  use_ull(i->b.seq);
+  return;
+}
Index: gcc/testsuite/ChangeLog
===================================================================
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 141763)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -3813,6 +3813,12 @@ rs6000_legitimize_address (rtx x, rtx ol
       HOST_WIDE_INT high_int, low_int;
       rtx sum;
       low_int = ((INTVAL (XEXP (x, 1)) & 0xffff) ^ 0x8000) - 0x8000;
+      /* Using a REG+CONST 64-bit integer load on 64-bit platforms
+	 requires that CONST be word-aligned.  */
+      if (TARGET_POWERPC64
+	  && (mode == DImode || mode == DDmode)
+	  && (low_int & 0x3))
+	low_int &= (HOST_WIDE_INT) ~3;
       high_int = INTVAL (XEXP (x, 1)) - low_int;
       sum = force_operand (gen_rtx_PLUS (Pmode, XEXP (x, 0),
 					 GEN_INT (high_int)), 0);

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[David Edelsohn]

On Tue, Nov 11, 2008 at 10:19 AM, Nathan Froyd <froydnj@codesourcery.com> wrote:

> Changed thusly.  OK?
>
> -Nathan
>
> gcc/
>        * config/rs6000/rs6000.c (rs6000_legitimize_address): Check for
>        non-word-aligned REG+CONST addressing.
>
> gcc/testsuite/
>        * gcc.c-torture/compile/20081111-1.c: New test.

This is okay with DDmode removed from the test.

Thanks, David

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[Alan Modra]

On Tue, Nov 11, 2008 at 07:19:11AM -0800, Nathan Froyd wrote:
> --- gcc/config/rs6000/rs6000.c	(revision 141763)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -3813,6 +3813,12 @@ rs6000_legitimize_address (rtx x, rtx ol
>        HOST_WIDE_INT high_int, low_int;
>        rtx sum;
>        low_int = ((INTVAL (XEXP (x, 1)) & 0xffff) ^ 0x8000) - 0x8000;
> +      /* Using a REG+CONST 64-bit integer load on 64-bit platforms
> +	 requires that CONST be word-aligned.  */
> +      if (TARGET_POWERPC64
> +	  && (mode == DImode || mode == DDmode)
> +	  && (low_int & 0x3))
> +	low_int &= (HOST_WIDE_INT) ~3;
>        high_int = INTVAL (XEXP (x, 1)) - low_int;
>        sum = force_operand (gen_rtx_PLUS (Pmode, XEXP (x, 0),
>  					 GEN_INT (high_int)), 0);

Is this really worth doing?  How does the generated code compare with
simply returning NULL from rs6000_legitimize_address for this case?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[Nathan Froyd]

On Wed, Nov 12, 2008 at 08:26:51AM +1030, Alan Modra wrote:
> On Tue, Nov 11, 2008 at 07:19:11AM -0800, Nathan Froyd wrote:
> > --- gcc/config/rs6000/rs6000.c	(revision 141763)
> > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > @@ -3813,6 +3813,12 @@ rs6000_legitimize_address (rtx x, rtx ol
> >        HOST_WIDE_INT high_int, low_int;
> >        rtx sum;
> >        low_int = ((INTVAL (XEXP (x, 1)) & 0xffff) ^ 0x8000) - 0x8000;
> > +      /* Using a REG+CONST 64-bit integer load on 64-bit platforms
> > +	 requires that CONST be word-aligned.  */
> > +      if (TARGET_POWERPC64
> > +	  && (mode == DImode || mode == DDmode)
> > +	  && (low_int & 0x3))
> > +	low_int &= (HOST_WIDE_INT) ~3;
> >        high_int = INTVAL (XEXP (x, 1)) - low_int;
> >        sum = force_operand (gen_rtx_PLUS (Pmode, XEXP (x, 0),
> >  					 GEN_INT (high_int)), 0);
> 
> Is this really worth doing?  How does the generated code compare with
> simply returning NULL from rs6000_legitimize_address for this case?

On a cross powerpc64 toolchain from our 4.3 sources (sorry, building a
native one takes forever here), the diffs between the code generated on
the included testcase looks like (MASK is from above code, NULL is what
I think you are proposing):

--- MASK.asm	2008-11-12 08:39:21.000000000 -0800
+++ NULL.asm	2008-11-12 08:39:06.000000000 -0800
@@ -14,8 +14,8 @@
 	std 0,16(1)
 	stdu 1,-112(1)
 	addis 3,3,0x1
-	addi 3,3,2
-	ld 3,-32768(3)
+	addi 3,3,-32766
+	ld 3,0(3)
 	addi 3,3,32764
 	bl use_ull
 	nop

-Nathan

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[Alan Modra]

On Wed, Nov 12, 2008 at 08:41:51AM -0800, Nathan Froyd wrote:
> On a cross powerpc64 toolchain from our 4.3 sources (sorry, building a
> native one takes forever here), the diffs between the code generated on
> the included testcase looks like (MASK is from above code, NULL is what
> I think you are proposing):
> 
> --- MASK.asm	2008-11-12 08:39:21.000000000 -0800
> +++ NULL.asm	2008-11-12 08:39:06.000000000 -0800
> @@ -14,8 +14,8 @@
>  	std 0,16(1)
>  	stdu 1,-112(1)
>  	addis 3,3,0x1
> -	addi 3,3,2
> -	ld 3,-32768(3)
> +	addi 3,3,-32766
> +	ld 3,0(3)
>  	addi 3,3,32764
>  	bl use_ull
>  	nop

This is what I expected to see, no benefit from trying to do something
special in rs6000_legitimize_address for these odd offsets.  I think
a better patch for the bug would be the following (totally untested!)

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 141807)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -3804,7 +3804,10 @@ rs6000_legitimize_address (rtx x, rtx ol
       && GET_CODE (XEXP (x, 0)) == REG
       && GET_CODE (XEXP (x, 1)) == CONST_INT
       && (unsigned HOST_WIDE_INT) (INTVAL (XEXP (x, 1)) + 0x8000) >= 0x10000
-      && !(SPE_VECTOR_MODE (mode)
+      && !((TARGET_POWERPC64
+	    && (mode == DImode || mode == TImode)
+	    && (INTVAL (XEXP (x, 1)) & 3) != 0)
+	   || SPE_VECTOR_MODE (mode)
 	   || ALTIVEC_VECTOR_MODE (mode)
 	   || (TARGET_E500_DOUBLE && (mode == DFmode || mode == TFmode
 				      || mode == DImode || mode == DDmode

You'll note that I've added TImode too.  This is to be consistent
with rs6000_legitimate_offset_address_p.  I haven't done the legwork
to prove that TImode accesses will hit this code in
rs6000_legitimize_address, but I strongly suspect they will.

BTW, your original patch would have broken with a TImode since
accessing the second dword would require [reg + reg + 8].

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH,rs6000] fix ICE caused by lax rs6000_legitimize_address

[David Edelsohn]

On Wed, Nov 12, 2008 at 8:22 PM, Alan Modra <amodra@bigpond.net.au> wrote:

> This is what I expected to see, no benefit from trying to do something
> special in rs6000_legitimize_address for these odd offsets.  I think
> a better patch for the bug would be the following (totally untested!)

Either approach will produce the same number of instructions and use
the same number of registers.  The difference is whether the remaining
displacement is zero or non-zero.  The new register value is unlikely to
be reused.  Either approach is okay with me.

Thanks, David