* [PATCH] PR c/8439
@ 2002-11-04 6:12 Eric Botcazou
2002-11-04 6:17 ` Paolo Carlini
2002-11-06 22:32 ` Mark Mitchell
0 siblings, 2 replies; 9+ messages in thread
From: Eric Botcazou @ 2002-11-04 6:12 UTC (permalink / raw)
To: gcc-patches
[-- 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++;
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-04 6:12 [PATCH] PR c/8439 Eric Botcazou
@ 2002-11-04 6:17 ` Paolo Carlini
2002-11-06 22:32 ` Mark Mitchell
1 sibling, 0 replies; 9+ messages in thread
From: Paolo Carlini @ 2002-11-04 6:17 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Mark Mitchell
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-04 6:12 [PATCH] PR c/8439 Eric Botcazou
2002-11-04 6:17 ` Paolo Carlini
@ 2002-11-06 22:32 ` Mark Mitchell
2002-11-08 4:41 ` Eric Botcazou
1 sibling, 1 reply; 9+ messages in thread
From: Mark Mitchell @ 2002-11-06 22:32 UTC (permalink / raw)
To: Eric Botcazou, gcc-patches
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-06 22:32 ` Mark Mitchell
@ 2002-11-08 4:41 ` Eric Botcazou
2002-11-08 9:07 ` Mark Mitchell
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2002-11-08 4:41 UTC (permalink / raw)
To: Mark Mitchell; +Cc: gcc-patches
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-08 4:41 ` Eric Botcazou
@ 2002-11-08 9:07 ` Mark Mitchell
2002-11-08 15:51 ` Eric Botcazou
2002-11-22 13:39 ` Eric Botcazou
0 siblings, 2 replies; 9+ messages in thread
From: Mark Mitchell @ 2002-11-08 9:07 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
> --- 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-08 9:07 ` Mark Mitchell
@ 2002-11-08 15:51 ` Eric Botcazou
2002-11-10 21:37 ` Mark Mitchell
2002-11-22 13:39 ` Eric Botcazou
1 sibling, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2002-11-08 15:51 UTC (permalink / raw)
To: Mark Mitchell; +Cc: gcc-patches
[-- 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++;
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-08 15:51 ` Eric Botcazou
@ 2002-11-10 21:37 ` Mark Mitchell
0 siblings, 0 replies; 9+ messages in thread
From: Mark Mitchell @ 2002-11-10 21:37 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
--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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-08 9:07 ` Mark Mitchell
2002-11-08 15:51 ` Eric Botcazou
@ 2002-11-22 13:39 ` Eric Botcazou
2002-11-22 13:48 ` Mark Mitchell
1 sibling, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2002-11-22 13:39 UTC (permalink / raw)
To: Mark Mitchell; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PR c/8439
2002-11-22 13:39 ` Eric Botcazou
@ 2002-11-22 13:48 ` Mark Mitchell
0 siblings, 0 replies; 9+ messages in thread
From: Mark Mitchell @ 2002-11-22 13:48 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
> OK for the branch, provided that it passes bootstrapping/regtesting ?
Yes.
--
Mark Mitchell mark@codesourcery.com
CodeSourcery, LLC http://www.codesourcery.com
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2002-11-22 21:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-11-04 6:12 [PATCH] PR c/8439 Eric Botcazou
2002-11-04 6:17 ` Paolo Carlini
2002-11-06 22:32 ` Mark Mitchell
2002-11-08 4:41 ` Eric Botcazou
2002-11-08 9:07 ` Mark Mitchell
2002-11-08 15:51 ` Eric Botcazou
2002-11-10 21:37 ` Mark Mitchell
2002-11-22 13:39 ` Eric Botcazou
2002-11-22 13:48 ` Mark Mitchell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).