[PR middle-end/20491] combine generates bad subregs

[PR middle-end/20491] combine generates bad subregs

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

Combine doesn't ensure the subregs it generates are valid.  In most
cases, insn recog will reject the invalid subregs, or reload will
somehow make them fit, but if the constraint of the insn or the asm
operand is "X", this won't work, so I think we're better off ensuring
we don't ever introduce subregs of non-REGs.

Does this look like a reasonable change to make?  If so, ok to install
if bootstrap and regtest on amd64-linux-gnu and i686-pc-linux-gnu
succeed?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combine-asm-subreg.patch --]
[-- Type: text/x-patch, Size: 2170 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* combine.c (subst): Make sure we don't create invalid subregs.

Index: gcc/combine.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/combine.c,v
retrieving revision 1.484
diff -u -p -r1.484 combine.c
--- gcc/combine.c 22 Mar 2005 03:48:44 -0000 1.484
+++ gcc/combine.c 24 Mar 2005 09:10:00 -0000
@@ -3689,15 +3689,22 @@ subst (rtx x, rtx from, rtx to, int in_d
 	      if (GET_CODE (new) == CLOBBER && XEXP (new, 0) == const0_rtx)
 		return new;
 
-	      if (GET_CODE (x) == SUBREG
-		  && (GET_CODE (new) == CONST_INT
-		      || GET_CODE (new) == CONST_DOUBLE))
+	      /* If we changed the reg of a subreg, make sure it's
+		 still valid.  For anything but a REG, require the
+		 SUBREG to be simplified out.  */
+
+	      if (GET_CODE (x) == SUBREG && new != SUBREG_REG (x))
 		{
 		  enum machine_mode mode = GET_MODE (x);
+		  enum machine_mode submode = GET_MODE (SUBREG_REG (x));
+
+		  if (GET_CODE (new) == REG)
+		    x = simplify_gen_subreg (mode, new, submode,
+					     SUBREG_BYTE (x));
+		  else
+		    x = simplify_subreg (mode, new, submode,
+					 SUBREG_BYTE (x));
 
-		  x = simplify_subreg (GET_MODE (x), new,
-				       GET_MODE (SUBREG_REG (x)),
-				       SUBREG_BYTE (x));
 		  if (! x)
 		    x = gen_rtx_CLOBBER (mode, const0_rtx);
 		}
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* gcc.dg/asm-subreg-1.c: New.

Index: gcc/testsuite/gcc.dg/asm-subreg-1.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/asm-subreg-1.c
diff -N gcc/testsuite/gcc.dg/asm-subreg-1.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/asm-subreg-1.c 24 Mar 2005 09:10:14 -0000
@@ -0,0 +1,14 @@
+/* PR middle-end/20491 */
+
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* Combine used to introduce invalid subregs for the asm input.  */
+
+volatile unsigned short _const_32 [4] = {1,2,3,4};
+void
+evas_common_convert_yuv_420p_601_rgba()
+{
+  __asm__ __volatile__ ("" : : "X" (*_const_32));
+}
+

[-- Attachment #3: Type: text/plain, Size: 188 bytes --]


-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [PR middle-end/20491] combine generates bad subregs

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On Mar 24, 2005, Alexandre Oliva <aoliva@redhat.com> wrote:

> Combine doesn't ensure the subregs it generates are valid.  In most
> cases, insn recog will reject the invalid subregs, or reload will
> somehow make them fit, but if the constraint of the insn or the asm
> operand is "X", this won't work, so I think we're better off ensuring
> we don't ever introduce subregs of non-REGs.

> Does this look like a reasonable change to make?

Ugh.  It failed building libgcc for stage 1, after simplifying
(subreg:QI (subreg:SI (reg:HI))) to (subreg:QI (reg:HI)), leaving
op0_mode set to SImode, causing an error in combine_simplify_rtx(),
when it called simplify_subreg with the wrong mode.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combine-asm-subreg.patch --]
[-- Type: text/x-patch, Size: 2181 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* combine.c (subst): Make sure we don't create invalid subregs.

Index: gcc/combine.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/combine.c,v
retrieving revision 1.484
diff -u -p -r1.484 combine.c
--- gcc/combine.c 22 Mar 2005 03:48:44 -0000 1.484
+++ gcc/combine.c 24 Mar 2005 10:01:17 -0000
@@ -3689,15 +3689,24 @@ subst (rtx x, rtx from, rtx to, int in_d
 	      if (GET_CODE (new) == CLOBBER && XEXP (new, 0) == const0_rtx)
 		return new;
 
-	      if (GET_CODE (x) == SUBREG
-		  && (GET_CODE (new) == CONST_INT
-		      || GET_CODE (new) == CONST_DOUBLE))
+	      /* If we changed the reg of a subreg, make sure it's
+		 still valid.  For anything but a REG, require the
+		 SUBREG to be simplified out.  */
+
+	      if (GET_CODE (x) == SUBREG && new != SUBREG_REG (x))
 		{
 		  enum machine_mode mode = GET_MODE (x);
+		  enum machine_mode submode = op0_mode;
+
+		  if (GET_CODE (new) == REG)
+		    x = simplify_gen_subreg (mode, new, submode,
+					     SUBREG_BYTE (x));
+		  else
+		    x = simplify_subreg (mode, new, submode,
+					 SUBREG_BYTE (x));
+
+		  op0_mode = VOIDmode;
 
-		  x = simplify_subreg (GET_MODE (x), new,
-				       GET_MODE (SUBREG_REG (x)),
-				       SUBREG_BYTE (x));
 		  if (! x)
 		    x = gen_rtx_CLOBBER (mode, const0_rtx);
 		}
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* gcc.dg/asm-subreg-1.c: New.

Index: gcc/testsuite/gcc.dg/asm-subreg-1.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/asm-subreg-1.c
diff -N gcc/testsuite/gcc.dg/asm-subreg-1.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/asm-subreg-1.c 24 Mar 2005 09:10:14 -0000
@@ -0,0 +1,14 @@
+/* PR middle-end/20491 */
+
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* Combine used to introduce invalid subregs for the asm input.  */
+
+volatile unsigned short _const_32 [4] = {1,2,3,4};
+void
+evas_common_convert_yuv_420p_601_rgba()
+{
+  __asm__ __volatile__ ("" : : "X" (*_const_32));
+}
+

[-- Attachment #3: Type: text/plain, Size: 188 bytes --]


-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [PR middle-end/20491] combine generates bad subregs

[Richard Henderson]

On Thu, Mar 24, 2005 at 07:45:44AM -0300, Alexandre Oliva wrote:
> 	* combine.c (subst): Make sure we don't create invalid subregs.

Ok.


r~

Re: [PR middle-end/20491] combine generates bad subregs

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Mar 28, 2005, Richard Henderson <rth@gcc.gnu.org> wrote:

> On Thu, Mar 24, 2005 at 07:45:44AM -0300, Alexandre Oliva wrote:
>> * combine.c (subst): Make sure we don't create invalid subregs.

> Ok.

As it turned out, the bug seems to have been fixed by some other
patch, because I no longer get the error in mainline or in the 4.0
branch.  Since my patch actually introduced a regression on
i686-pc-linux-gnu: gcc.dg/i386-rotate-1.c.  Because of the ruled-out
combines, we prevented the complete transformation from taking place.
I actually wrote another, more complex patch, that fixed it, by using
the same machinery used by later portions of combine to push the
invalid subreg into the shift operands, i.e., turn (subreg (ashift
(reg) (const_int))) into (ashift (subreg (reg)) (const_int)), but then
I decided I was working too hard and figured I might be better off
fixing the problem elsewhere.  To my surprise, after reverting the
patch I had, the problem no longer occurred.  I tried some CVS
searching, but couldn't locate the patch that fixed the problem.  In
fact, I couldn't even find a relatively-recent tree that triggered the
bug, although I'm pretty sure the tree in which I triggered the bug
was no older than some date early last week.  Oh well...

I'm attaching the patch I had, in case you still think we should avoid
creating such invalid subregs, followed by the testcase patch I
installed in the 4.0 branch in mainline.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combine-asm-subreg-take3.patch --]
[-- Type: text/x-patch, Size: 1785 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* combine.c (subst): Make sure we don't create invalid subregs.

Index: gcc/combine.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/combine.c,v
retrieving revision 1.484
diff -u -p -r1.484 combine.c
--- gcc/combine.c 22 Mar 2005 03:48:44 -0000 1.484
+++ combine.c	25 Mar 2005 21:21:40 -0000
@@ -3689,15 +3689,36 @@ subst (rtx x, rtx from, rtx to, int in_d
 	      if (GET_CODE (new) == CLOBBER && XEXP (new, 0) == const0_rtx)
 		return new;
 
+	      /* If we changed the reg of a subreg, make sure it's
+		 still valid.  For anything but a REG, require the
+		 SUBREG to be simplified out.  */
+
 	      if (GET_CODE (x) == SUBREG
-		  && (GET_CODE (new) == CONST_INT
-		      || GET_CODE (new) == CONST_DOUBLE))
+		  && ! rtx_equal_p (new, SUBREG_REG (x)))
 		{
 		  enum machine_mode mode = GET_MODE (x);
+		  rtx orig = x;
+
+		  x = simplify_gen_subreg (mode, new, op0_mode,
+					   SUBREG_BYTE (orig));
+
+		  /* This will propagate the subreg into the operand,
+		     if appropriate.  */
+		  if (GET_CODE (x) == SUBREG
+		      && GET_CODE (SUBREG_REG (x)) != REG)
+		    x = force_to_mode (x, mode, GET_MODE_MASK (mode),
+				       NULL_RTX, 0);
+
+		  /* Now make sure we didn't create a subreg of
+		     something that is not a reg.  */
+		  if (GET_CODE (x) == SUBREG
+		      && GET_CODE (SUBREG_REG (x)) != REG)
+		    x = simplify_subreg (mode, SUBREG_REG (x),
+					 GET_MODE (SUBREG_REG (x)),
+					 SUBREG_BYTE (x));
+
+		  op0_mode = VOIDmode;
 
-		  x = simplify_subreg (GET_MODE (x), new,
-				       GET_MODE (SUBREG_REG (x)),
-				       SUBREG_BYTE (x));
 		  if (! x)
 		    x = gen_rtx_CLOBBER (mode, const0_rtx);
 		}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: combine-asm-subreg-test.patch --]
[-- Type: text/x-patch, Size: 839 bytes --]

Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* gcc.dg/torture/asm-subreg-1.c: New test.

Index: gcc/testsuite/gcc.dg/torture/asm-subreg-1.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/torture/asm-subreg-1.c
diff -N gcc/testsuite/gcc.dg/torture/asm-subreg-1.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/torture/asm-subreg-1.c 29 Mar 2005 21:31:55 -0000
@@ -0,0 +1,14 @@
+/* PR middle-end/20491 */
+
+/* { dg-do compile } */
+
+/* Combine used to introduce invalid subregs for the asm input, and
+   we'd crash later on, when removing all subregs.  */
+
+volatile unsigned short _const_32 [4] = {1,2,3,4};
+void
+evas_common_convert_yuv_420p_601_rgba()
+{
+  __asm__ __volatile__ ("" : : "X" (*_const_32));
+}
+

[-- Attachment #4: Type: text/plain, Size: 189 bytes --]



-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [PR middle-end/20491] combine generates bad subregs

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On Mar 29, 2005, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Mar 28, 2005, Richard Henderson <rth@gcc.gnu.org> wrote:
>> On Thu, Mar 24, 2005 at 07:45:44AM -0300, Alexandre Oliva wrote:
>>> * combine.c (subst): Make sure we don't create invalid subregs.

>> Ok.

> As it turned out, the bug seems to have been fixed by some other
> patch, because I no longer get the error in mainline or in the 4.0
> branch.

Or perhaps it never actually failed on x86_64-linux-gnu, which is
where I was trying to trigger it again.  Not even with -m32.  A build
on i686-pc-linux-gnu was enough to trigger it.  Yay me! :-)

So, the patch posted in my previous e-mail still stands, but if you,
like me, find that it's working too hard to avoid such subregs, here's
an alternative patch that also fixes the problem, at least as long as
the X-constrained asm operand is not actually used in the asm output
pattern.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combine-asm-subreg-take4.patch --]
[-- Type: text/x-patch, Size: 1026 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* final.c (alter_subreg): Don't call subreg_regno for a non-REG.

Index: gcc/final.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/final.c,v
retrieving revision 1.344.12.1
diff -u -p -r1.344.12.1 final.c
--- gcc/final.c 22 Mar 2005 13:39:12 -0000 1.344.12.1
+++ gcc/final.c 30 Mar 2005 19:21:02 -0000
@@ -1,6 +1,7 @@
 /* Convert RTL to assembler code and output it, for GNU compiler.
    Copyright (C) 1987, 1988, 1989, 1992, 1993, 1994, 1995, 1996, 1997,
-   1998, 1999, 2000, 2001, 2002, 2003, 2004 Free Software Foundation, Inc.
+   1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005
+     Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -2638,7 +2639,7 @@ alter_subreg (rtx *xp)
 
       if (new != 0)
 	*xp = new;
-      else
+      else if (REG_P (y))
 	{
 	  /* Simplify_subreg can't handle some REG cases, but we have to.  */
 	  unsigned int regno = subreg_regno (x);

[-- Attachment #3: Type: text/plain, Size: 188 bytes --]


-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [PR middle-end/20491] combine generates bad subregs

[Richard Henderson]

On Wed, Mar 30, 2005 at 04:27:50PM -0300, Alexandre Oliva wrote:
> -      else
> +      else if (REG_P (y))
>  	{
>  	  /* Simplify_subreg can't handle some REG cases, but we have to.  */
>  	  unsigned int regno = subreg_regno (x);

The next line is 

          gcc_assert (REG_P (y));

you should remove that.  Ok with that change.


r~

Re: [PR middle-end/20491] combine generates bad subregs

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On Mar 31, 2005, Richard Henderson <rth@gcc.gnu.org> wrote:

> On Wed, Mar 30, 2005 at 04:27:50PM -0300, Alexandre Oliva wrote:
>> -      else
>> +      else if (REG_P (y))
>> {
>> /* Simplify_subreg can't handle some REG cases, but we have to.  */
>> unsigned int regno = subreg_regno (x);

> The next line is 

>           gcc_assert (REG_P (y));

> you should remove that.  Ok with that change.

Thanks, here's what I'm checking in.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: combine-asm-subreg.patch --]
[-- Type: text/x-patch, Size: 772 bytes --]

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR middle-end/20491
	* final.c (alter_subreg): Don't call subreg_regno for a non-REG.

Index: gcc/final.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/final.c,v
retrieving revision 1.349
diff -u -p -r1.349 final.c
--- gcc/final.c	1 Apr 2005 15:27:58 -0000	1.349
+++ gcc/final.c	1 Apr 2005 20:19:02 -0000
@@ -2547,11 +2547,10 @@ alter_subreg (rtx *xp)
 
       if (new != 0)
 	*xp = new;
-      else
+      else if (REG_P (y))
 	{
 	  /* Simplify_subreg can't handle some REG cases, but we have to.  */
 	  unsigned int regno = subreg_regno (x);
-	  gcc_assert (REG_P (y));
 	  *xp = gen_rtx_REG_offset (y, GET_MODE (x), regno, SUBREG_BYTE (x));
 	}
     }

[-- Attachment #3: Type: text/plain, Size: 188 bytes --]


-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [PR middle-end/20491] combine generates bad subregs

[Alexandre Oliva]

On Mar 24, 2005, kenner@vlsi1.ultra.nyu.edu (Richard Kenner) wrote:

>     Combine doesn't ensure the subregs it generates are valid.  In most
>     cases, insn recog will reject the invalid subregs, or reload will
>     some how make them fit, but if the constraint of the insn or the asm
>     operand is "X", this won't work, so I think we're better off ensuring
>     we don't ever introduce subregs of non-REGs.

> Aside from calling recog combine doesn't check that *anything* it generates
> is valid.  Why should subregs be different?

Because, as I said, generating an invalid subreg at that point,
without any opportunity to fix it up later, will cause us to crash
when the time comes to remove all subregs.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: [PR middle-end/20491] combine generates bad subregs

[Richard Kenner]

    Combine doesn't ensure the subregs it generates are valid.  In most
    cases, insn recog will reject the invalid subregs, or reload will
    some how make them fit, but if the constraint of the insn or the asm
    operand is "X", this won't work, so I think we're better off ensuring
    we don't ever introduce subregs of non-REGs.

Aside from calling recog combine doesn't check that *anything* it generates
is valid.  Why should subregs be different?