[PATCH] PR c/8439

[PATCH] PR c/8439

[Eric Botcazou]

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

This is a high-priority PR, regression both on the mainline and the branch. 
The compiler ICEs in instantiate_virtual_regs_1 on the following insn:

(insn 9 6 10 (parallel[ 
            (set (mem/f:SI (reg/f:SI 53 virtual-incoming-args) [0 p+0 S4 A32])
                (plus:SI (mem/f:SI (reg/f:SI 53 virtual-incoming-args) [0 p+0 
S4 A32])
                    (const_int 0 [0x0])))
            (clobber (reg:CC 17 flags))
        ] ) -1 (nil)
    (nil))

because the recognizer again chokes on the (plus (mem) (const_int 0)) pattern.

The proposed fix is to teach expand_increment not to emit insn when the 
increment is null. Bootstrapped/regtested (c,c++,objc,f77 mainline) on 
i586-redhat-linux-gnu.


2002-11-04  Eric Botcazou  <ebotcazou@libertysurf.fr>

	PR c/8439
	* expr.c (expand_increment): Don't emit insn if the
	increment is null.

-- 
Eric Botcazou

[-- Attachment #2: pr8439.diff --]
[-- Type: text/x-diff, Size: 764 bytes --]

--- gcc/expr.c.orig	Sun Nov  3 23:25:36 2002
+++ gcc/expr.c	Sun Nov  3 23:26:41 2002
@@ -9388,6 +9388,9 @@
 
   temp = get_last_insn ();
   op0 = expand_expr (incremented, NULL_RTX, VOIDmode, 0);
+  op1 = expand_expr (TREE_OPERAND (exp, 1), NULL_RTX, VOIDmode, 0);
+  if (op1 == const0_rtx)
+    return op0;  
 
   /* If OP0 is a SUBREG made for a promoted variable, we cannot increment
      in place but instead must do sign- or zero-extension during assignment,
@@ -9418,7 +9421,6 @@
 
   op0_is_copy = ((GET_CODE (op0) == SUBREG || GET_CODE (op0) == REG)
 		 && temp != get_last_insn ());
-  op1 = expand_expr (TREE_OPERAND (exp, 1), NULL_RTX, VOIDmode, 0);
 
   /* Decide whether incrementing or decrementing.  */
   if (TREE_CODE (exp) == POSTDECREMENT_EXPR

[-- Attachment #3: pr8439.c --]
[-- Type: text/x-csrc, Size: 130 bytes --]

/* PR c/8439 */
/* Verify that GCC properly handles null increments. */

struct empty {
};

void foo(struct empty *p)
{
   p++;
}

Re: [PATCH] PR c/8439

[Paolo Carlini]

Eric Botcazou wrote:

>This is a high-priority PR, regression both on the mainline and the branch.
>
Thanks for working on this one! I triaged it yesterday and found quite 
irritating.

I hope the fix will find its way in 3.2.1 too.

Ciao, Paolo.

Re: [PATCH] PR c/8439

[Mark Mitchell]

> The proposed fix is to teach expand_increment not to emit insn when the
> increment is null. Bootstrapped/regtested (c,c++,objc,f77 mainline) on
> i586-redhat-linux-gnu.

This fix isn't correct; if the thing being incremented is volatile, we
should still access it.

The real problem is that validate_replace_rtx_1 is performing
simplifications; when we replace the virtual argument pointer with
the real one it tries to get rid of the plus in the following:

(set (mem/f:SI (reg/f:SI 53 virtual-incoming-args) [0 p+0 S4 A32])
  (plus:SI (mem/f:SI (reg/f:SI 53 virtual-incoming-args) [0 p+0 S4 A32])
    (const_int 0 [0x0])))

which makes the expression unrecognizable.

There's even a comment:

      /* If we have a PLUS whose second operand is now a CONST_INT, use
         plus_constant to try to simplify it.
         ??? We may want later to remove this, once simplification is
         separated from this function.  */

We should really be simplifying only if *both* arguments are now
CONST_INTs.  I'll try that for mainline.

I thought about trying to fix this as you suggested, or even earlier
in the production of trees, but the volatile thing makes it a bit
tricky, and the bottom line is that validate_replace_rtx_1 is broken.

So, we'll punt this one to another day; I can't see messing with anything
in the generic middle end code to fix a bug involving the GNU zero-sized
structure extension.  (Insert rant about extensions here.)

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: [PATCH] PR c/8439

[Eric Botcazou]

> The real problem is that validate_replace_rtx_1 is performing
> simplifications; when we replace the virtual argument pointer with
> the real one it tries to get rid of the plus in the following:
>
> (set (mem/f:SI (reg/f:SI 53 virtual-incoming-args) [0 p+0 S4 A32])
>   (plus:SI (mem/f:SI (reg/f:SI 53 virtual-incoming-args) [0 p+0 S4 A32])
>     (const_int 0 [0x0])))
>
> which makes the expression unrecognizable.

Yes, the (const_int 0) is thrown away. This seems to be really problematic 
(see PR c/7411).

> There's even a comment:
>
>       /* If we have a PLUS whose second operand is now a CONST_INT, use
>          plus_constant to try to simplify it.
>          ??? We may want later to remove this, once simplification is
>          separated from this function.  */
>
> We should really be simplifying only if *both* arguments are now
> CONST_INTs.  I'll try that for mainline.

Perhaps we should be simplifying only if there is something new to be 
simplified ? This approach had been used until Jan's patch:
http://gcc.gnu.org/ml/gcc-patches/2001-06/msg00780.html

After reading the message, I don't know whether the patch is a fix or not.
Anyway, reverting it fixes the problem.

--- gcc/recog.c.orig    Fri Nov  8 12:41:35 2002
+++ gcc/recog.c Fri Nov  8 13:13:22 2002
@@ -522,10 +522,10 @@
     {
     case PLUS:
       /* If we have a PLUS whose second operand is now a CONST_INT, use
-         plus_constant to try to simplify it.
+         simplify_gen_binary to try to simplify it.
          ??? We may want later to remove this, once simplification is
          separated from this function.  */
-      if (GET_CODE (XEXP (x, 1)) == CONST_INT)
+      if (GET_CODE (XEXP (x, 1)) == CONST_INT && XEXP (x, 1) == to)
        validate_change (object, loc,
                         simplify_gen_binary
                         (PLUS, GET_MODE (x), XEXP (x, 0), XEXP (x, 1)), 1);

I can bootstrap/regtest it if you think this approach is correct (even on the 
branch).

-- 
Eric Botcazou

Re: [PATCH] PR c/8439

[Mark Mitchell]

> --- gcc/recog.c.orig    Fri Nov  8 12:41:35 2002
> +++ gcc/recog.c Fri Nov  8 13:13:22 2002
> @@ -522,10 +522,10 @@
>      {
>      case PLUS:
>        /* If we have a PLUS whose second operand is now a CONST_INT, use
> -         plus_constant to try to simplify it.
> +         simplify_gen_binary to try to simplify it.
>           ??? We may want later to remove this, once simplification is
>           separated from this function.  */
> -      if (GET_CODE (XEXP (x, 1)) == CONST_INT)
> +      if (GET_CODE (XEXP (x, 1)) == CONST_INT && XEXP (x, 1) == to)
>         validate_change (object, loc,
>                          simplify_gen_binary
>                          (PLUS, GET_MODE (x), XEXP (x, 0), XEXP (x, 1)),
> 1);
>
> I can bootstrap/regtest it if you think this approach is correct (even on
> the  branch).

That seems like a good strategy.  Try it, and check it in if it passes.

But put it only on the mainline; if we do a GCC 3.2.2 we can consider
moving it then.

Thanks!

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: [PATCH] PR c/8439

[Eric Botcazou]

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

> That seems like a good strategy.  Try it, and check it in if it passes.

Bootstrapped/regtested (c,c++,objc,f77 mainline) on i586-redhat-linux-gnu.
I don't have CVS access, so you need to check it in for me. Patch and testcase 
attached.


2002-11-09  Eric Botcazou  <ebotcazou@libertysurf.fr>

	PR c/8439
	* recog.c (validate_replace_rtx_1) [PLUS]: Simplify only
	if there is something new to be simplified.


In light of this patch, the fix I proposed for PR c/7411 was only papering over 
the problem and is now useless (it taught expand_expr to throw away null operands 
in PLUS expressions). Do you want me to submit a patch that reverts it ?

-- 
Eric Botcazou

[-- Attachment #2: pr8439.diff --]
[-- Type: text/x-diff, Size: 651 bytes --]

--- gcc/recog.c.orig	Fri Nov  8 12:41:35 2002
+++ gcc/recog.c	Fri Nov  8 13:13:22 2002
@@ -522,10 +522,10 @@
     {
     case PLUS:
       /* If we have a PLUS whose second operand is now a CONST_INT, use
-         plus_constant to try to simplify it.
+         simplify_gen_binary to try to simplify it.
          ??? We may want later to remove this, once simplification is
          separated from this function.  */
-      if (GET_CODE (XEXP (x, 1)) == CONST_INT)
+      if (GET_CODE (XEXP (x, 1)) == CONST_INT && XEXP (x, 1) == to)
 	validate_change (object, loc,
 			 simplify_gen_binary
 			 (PLUS, GET_MODE (x), XEXP (x, 0), XEXP (x, 1)), 1);

[-- Attachment #3: pr8439.c --]
[-- Type: text/x-csrc, Size: 130 bytes --]

/* PR c/8439 */
/* Verify that GCC properly handles null increments. */

struct empty {
};

void foo(struct empty *p)
{
   p++;
}

Re: [PATCH] PR c/8439

[Mark Mitchell]

--On Saturday, November 09, 2002 12:47:14 AM +0100 Eric Botcazou 
<ebotcazou@libertysurf.fr> wrote:

>> That seems like a good strategy.  Try it, and check it in if it passes.
>
> Bootstrapped/regtested (c,c++,objc,f77 mainline) on i586-redhat-linux-gnu.
> I don't have CVS access, so you need to check it in for me. Patch and
> testcase  attached.

Committed to mainline.  If you're going to do more patching (and I hope you
will!), please request a CVS account.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: [PATCH] PR c/8439

[Eric Botcazou]

On Friday 08 November 2002 18:04, Mark Mitchell wrote:
> > --- gcc/recog.c.orig    Fri Nov  8 12:41:35 2002
> > +++ gcc/recog.c Fri Nov  8 13:13:22 2002
> > @@ -522,10 +522,10 @@
> >      {
> >      case PLUS:
> >        /* If we have a PLUS whose second operand is now a CONST_INT, use
> > -         plus_constant to try to simplify it.
> > +         simplify_gen_binary to try to simplify it.
> >           ??? We may want later to remove this, once simplification is
> >           separated from this function.  */
> > -      if (GET_CODE (XEXP (x, 1)) == CONST_INT)
> > +      if (GET_CODE (XEXP (x, 1)) == CONST_INT && XEXP (x, 1) == to)
> >         validate_change (object, loc,
> >                          simplify_gen_binary
> >                          (PLUS, GET_MODE (x), XEXP (x, 0), XEXP (x, 1)),
> > 1);
> >
> > I can bootstrap/regtest it if you think this approach is correct (even on
> > the  branch).
>
> That seems like a good strategy.  Try it, and check it in if it passes.
>
> But put it only on the mainline; if we do a GCC 3.2.2 we can consider
> moving it then.

OK for the branch, provided that it passes bootstrapping/regtesting ?

-- 
Eric Botcazou

Re: [PATCH] PR c/8439

[Mark Mitchell]

> OK for the branch, provided that it passes bootstrapping/regtesting ?

Yes.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com