Bug 13985 - [3.4/4.0 regression] ICE in gcc.c-torture/compile/930621-1.c
[3.4/4.0 regression] ICE in gcc.c-torture/compile/930621-1.c
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: rtl-optimization
3.4.0
: P2 normal
: 3.4.1
Assigned To: Eric Botcazou
: ice-on-valid-code, patch
: 15512 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-02-02 21:35 UTC by cato
Modified: 2004-09-13 14:15 UTC (History)
7 users (show)

See Also:
Host: i386-*-*
Target: i386-*-*
Build: i386-*-*
Known to work:
Known to fail:
Last reconfirmed: 2004-02-02 23:35:35


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description cato 2004-02-02 21:35:00 UTC
The gcc.c-torture/compile/930621-1.c ICE when using -O3 for
i386-unknown-netbsdelf1.6 on mainline and the 3.4 branch.


The failure (compiled from today's mainline):
  /tmp/build/gcc/xgcc -B/tmp/build/gcc -O3 -c 930621-1.c
  930621-1.c: In function `modlookup':
  930621-1.c:38: internal compiler error: in get_loop_body, at cfgloop.c:958
  Please submit a full bug report,
  with preprocessed source if appropriate.
  See <URL:http://gcc.gnu.org/bugs.html> for instructions.

Compiler version:
  /tmp/build/gcc/xgcc -v
  Using built-in specs.
  Configured with: ../gcc/configure --disable-werror
  Thread model: single
  gcc version 3.5.0 20040202 (experimental)


The commit that broke this test is:
  CVSROOT:        /cvs/gcc
  Module name:    gcc
  Changes by:     steven@gcc.gnu.org      2004-01-05 09:35:06

  Modified files:
          gcc            : toplev.c 

  Log message:
          I am a moron.

  Patches:
 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/toplev.c.diff?cvsroot=gcc&r1=1.857&r2=1.858
Comment 1 Steven Bosscher 2004-02-02 23:35:34 UTC
This checkin of mine is a good candidate for the worst checkin ever.  The fix 
has been on mainline for a couple of days now, and I'll put it on the 3.4 
branch later today. 
Comment 2 Steven Bosscher 2004-02-03 00:53:21 UTC
This checking should fix it: 
http://gcc.gnu.org/ml/gcc-cvs/2004-02/msg00086.html 
 
Can you confirm that please? 
 
Comment 3 cato 2004-02-03 05:51:11 UTC
Subject: Re:  [3.4/3.5 regression] ICE in
 gcc.c-torture/compile/930621-1.c


On Tue, 2 Feb 2004, steven at gcc dot gnu dot org wrote:

> ------- Additional Comments From steven at gcc dot gnu dot org  2004-02-02 23:35 -------
> This checkin of mine is a good candidate for the worst checkin ever.  The fix
> has been on mainline for a couple of days now, and I'll put it on the 3.4
> branch later today.

The problem is still present on mainline -- the test did fail when I last
bootstrapped and tested, 10 hours ago...

   /Krister
Comment 4 Steven Bosscher 2004-02-03 08:39:02 UTC
Subject: Re:  [3.4/3.5 regression] ICE in gcc.c-torture/compile/930621-1.c

On Tuesday 03 February 2004 06:51, cato at df dot lth dot se wrote:
> The problem is still present on mainline -- the test did fail when I last
> bootstrapped and tested, 10 hours ago...

Then my broken checkin cannot be the cause of this because it has been 
corrected on mainline several days ago.  Something must have broken it
before December 30, 2003.

Comment 5 Steven Bosscher 2004-02-03 08:59:45 UTC
Indeed, according to the posted test results, this was already 
failing before December 30, 2003.  In fact it also FAILed in 
October.  See the following two messages: 
http://gcc.gnu.org/ml/gcc-testresults/2003-12/msg00853.html 
http://gcc.gnu.org/ml/gcc-testresults/2003-10/msg01343.html 
 
So this is not a new FAIL, just one that was hidden because 
of a broken checkin that moved loop2. 
Comment 6 cato 2004-02-03 23:01:31 UTC
Subject: Re:  [3.4/3.5 regression] ICE in
 gcc.c-torture/compile/930621-1.c


On Tue, 3 Feb 2004, steven at gcc dot gnu dot org wrote:

> So this is not a new FAIL, just one that was hidden because
> of a broken checkin that moved loop2.

OK. I'll find out what broke it the first time...
Comment 7 cato 2004-02-07 01:42:57 UTC
Subject: Re:  [3.4/3.5 regression] ICE in
 gcc.c-torture/compile/930621-1.c

The change that introduced this failure is:

2003-10-25  Roger Sayle  <roger@eyesopen.com>

       * simplify-rtx.c (simplify_replace_rtx): Avoid allocating duplicate
       RTL nodes.  If an operator's operands are unchanged, return the
       original argument unchanged.
Comment 8 Andrew Pinski 2004-02-07 02:19:15 UTC
Reopening based on it is not fixed.
Comment 9 Andrew Pinski 2004-02-07 02:22:14 UTC
Roger this is caused by the same patch as before, but this is for i386 instead of i686.
Comment 10 Andrew Pinski 2004-03-04 01:57:23 UTC
Roger are you still working on this, I think this is not yours though.
Comment 11 Eric Botcazou 2004-03-06 11:36:28 UTC
Investigating.
Comment 12 Eric Botcazou 2004-03-11 06:31:03 UTC
Patch: http://gcc.gnu.org/ml/gcc-patches/2004-03/msg00672.html
Comment 13 Mark Mitchell 2004-03-21 18:56:00 UTC
I keep looking at this patch, and I keep not knowing whether I know enough to
approve it.

Eric, would you run this patch past RTH and/or Jeff Law and see what their
reaction is?

If they think it's OK for 3.4.0, put it in.  Otherwise, let's move this back to
3.4.1.
Comment 14 Mark Mitchell 2004-04-18 21:53:59 UTC
It appears that Richard/Jeff were either never asked or never approved this
patch.  Therefore, I've postponed it until GCC 3.4.1.
Comment 15 Eric Botcazou 2004-04-19 05:29:08 UTC
For the sake of completeness: they were both asked.
Comment 16 Jeffrey A. Law 2004-04-19 16:25:32 UTC
Subject: Re:  [3.4/3.5 regression] ICE in 
 gcc.c-torture/compile/930621-1.c

In message <20040419052909.23755.qmail@sources.redhat.com>, "ebotcazou at gcc d
ot gnu dot org" writes:
 >
 >------- Additional Comments From ebotcazou at gcc dot gnu dot org  2004-04-19
 > 05:29 -------
 >For the sake of completeness: they were both asked.
Sorry, I must have missed that email message (Eric certainly sent it back in
March).

The patch looks reasonable to me -- though I must admit, I'm not very 
familiar with the code in cfgloopmanip.c.

You might ping Zdenek directly for a sanity check -- he wrote much of that
code originally and is pretty sharp regarding updates of datastructures
derived from the CFG such as the loop tree.

jeff

Comment 17 Eric Botcazou 2004-04-19 21:27:36 UTC
Thanks Jeff.

Zdenek, would you mind taking a look at the patch linked to in comment #12?  Do
you think the problem is correctly assessed?  If so, do you think it's the right
solution to the problem?  Thanks in advance.
Comment 18 Zdenek Dvorak 2004-04-19 22:53:01 UTC
Subject: Re:  [3.4/3.5 regression] ICE in gcc.c-torture/compile/930621-1.c

Hello,

> Zdenek, would you mind taking a look at the patch linked to in comment #12?  Do
> you think the problem is correctly assessed?  If so, do you think it's the right
> solution to the problem?  Thanks in advance.

the patch seems OK to me (good work -- I was not really able to write this part of
loop manipulation in a clear way :-( ).  Perhaps I would suggest to formulate the
comment

+      /* Changing the placement of a loop in the loop tree may have an
+        effect on its preheader with regard to the condition stated in
+        the description of fix_bb_placement.  */
+      fix_bb_placements (loops, loop_preheader_edge (loop)->src);

a bit more precisely:

We have determined that LOOP is no longer a subloop of its original
LOOP->FATHER.  This means that we removed some edge(s) and thus
cancelled all paths from the LOOP to LOOP->FATHER's latch.  This might
however cause also cancelation of all paths from the preheader of the
loop (and possibly some of its predecessors) to this latch, so we need
to update their placement inside loops as well.

Zdenek
Comment 19 Eric Botcazou 2004-04-20 06:48:46 UTC
Thanks Zdenek.

However, as a GCC hacker who didn't know anything about the code in
cfgloopmanip.c before trying to fix the bug, I think your comment is too
detailed(!) and assume too much of a familiarity with the cfg*.c modules.

I think we must comment on the same level of abstraction as the function itself.
 The description of fix_bb_placement is nice:

/* Fix placement of basic block BB inside loop hierarchy stored in LOOPS --
   Let L be a loop to that BB belongs.  Then every successor of BB must either
     1) belong to some superloop of loop L, or
     2) be a header of loop K such that K->outer is superloop of L
   Returns true if we had to move BB into other loop to enforce this condition,
   false if the placement of BB was already correct (provided that placements
   of its successors are correct).  */

I think it is obvious to see (well, modulo a few seconds :-) that if a loop is
reparented, condition 2) is altered for its preheader.  You don't need to know
anything about the latch and the search order.

What about the following wording instead?

/* Changing the placement of a loop in the loop tree may alter the validity
   of condition 2) stated in the description of fix_bb_placement for its
   preheader, because the successor is the header and belongs to the loop.
   So call fix_bb_placements to fix up the placement of the preheader and
   (possibly) of its predecessors.  */


[As you have probably guessed, this whole comment is primarily aimed at being
considered as a nominee for the next hair-splitting contest :-)]
Comment 20 Zdenek Dvorak 2004-04-20 07:18:42 UTC
Subject: Re:  [3.4/3.5 regression] ICE in gcc.c-torture/compile/930621-1.c

Hello,

> However, as a GCC hacker who didn't know anything about the code in
> cfgloopmanip.c before trying to fix the bug, I think your comment is too
> detailed(!) and assume too much of a familiarity with the cfg*.c modules.

I don't understand?  The comment as I formulated it assumes very little
knowledge about our implementation (basically just that loop->father is
superloop of the loop).  It only refers to the well-known definition of
the natural loop.

> I think we must comment on the same level of abstraction as the function itself.

I was sort of thinking about extending the comments at the
fix_{bb,loop}_placement in a similar way (i.e. to explain not only what
they are doing, but also why it is so).

>  The description of fix_bb_placement is nice:
> /* Fix placement of basic block BB inside loop hierarchy stored in LOOPS --
>    Let L be a loop to that BB belongs.  Then every successor of BB must either
>      1) belong to some superloop of loop L, or
>      2) be a header of loop K such that K->outer is superloop of L
>    Returns true if we had to move BB into other loop to enforce this condition,
>    false if the placement of BB was already correct (provided that placements
>    of its successors are correct).  */
> 
> I think it is obvious to see (well, modulo a few seconds :-) that if a loop is
> reparented, condition 2) is altered for its preheader.  You don't need to know
> anything about the latch and the search order.
> 
> What about the following wording instead?
> 
> /* Changing the placement of a loop in the loop tree may alter the validity
>    of condition 2) stated in the description of fix_bb_placement for its
>    preheader, because the successor is the header and belongs to the loop.
>    So call fix_bb_placements to fix up the placement of the preheader and
>    (possibly) of its predecessors.  */
> 
> 
> [As you have probably guessed, this whole comment is primarily aimed at being
> considered as a nominee for the next hair-splitting contest :-)]

Zdenek
Comment 21 Eric Botcazou 2004-04-20 09:24:25 UTC
> I don't understand?  The comment as I formulated it assumes very little
> knowledge about our implementation (basically just that loop->father is
> superloop of the loop).  It only refers to the well-known definition of
> the natural loop.

That loop->father is a superloop of the loop is hardly problematic indeed :-)

But "all paths from the LOOP to LOOP->FATHER's latch" implicitly assumes that
you know the criterion based on which the loop are sorted and, given the lack of
 hurry to approve the patch :-), I don't think everyone is as familiar as you
with this stuff.

> I was sort of thinking about extending the comments at the
> fix_{bb,loop}_placement in a similar way (i.e. to explain not only what
> they are doing, but also why it is so).

IMHO such a general comment would rather belong in the class of file-scope
comments (that cfgloopmanip.c lacks btw).  As I said, the comment of
fix_bb_placement is nice because it matches the level of abstraction of the
function itself (which doesn't deal with latches).
Comment 22 CVS Commits 2004-04-23 22:01:44 UTC
Subject: Bug 13985

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	ebotcazou@gcc.gnu.org	2004-04-23 22:01:36

Modified files:
	gcc            : ChangeLog cfgloopmanip.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: loop-3.c 

Log message:
	PR optimization/13985
	* cfgloopmanip.c (fix_loop_placements): New prototype.
	Call fix_bb_placements on the preheader of loops that have
	been reparented.
	(remove_path): Adjust call to fix_loop_placements.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.3473&r2=2.3474
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cfgloopmanip.c.diff?cvsroot=gcc&r1=1.24&r2=1.25
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3699&r2=1.3700
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/loop-3.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 23 CVS Commits 2004-04-23 22:10:50 UTC
Subject: Bug 13985

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	ebotcazou@gcc.gnu.org	2004-04-23 22:10:47

Modified files:
	gcc            : ChangeLog cfgloopmanip.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.dg: loop-3.c 

Log message:
	PR optimization/13985
	* cfgloopmanip.c (fix_loop_placements): New prototype.
	Call fix_bb_placements on the preheader of loops that have
	been reparented.
	(remove_path): Adjust call to fix_loop_placements.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.415&r2=2.2326.2.416
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cfgloopmanip.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.19&r2=1.19.4.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3389.2.172&r2=1.3389.2.173
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.dg/loop-3.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.2.1

Comment 24 Eric Botcazou 2004-04-23 22:12:32 UTC
Fixed, finally.
Comment 25 Eric Botcazou 2004-05-18 11:19:30 UTC
*** Bug 15512 has been marked as a duplicate of this bug. ***