This is GCC Bugzilla
This is GCC Bugzilla Version 2.20+
View Bug Activity | Format For Printing | Clone This Bug
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
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.
This checking should fix it: http://gcc.gnu.org/ml/gcc-cvs/2004-02/msg00086.html Can you confirm that please?
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
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.
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.
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...
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.
Reopening based on it is not fixed.
Roger this is caused by the same patch as before, but this is for i386 instead of i686.
Roger are you still working on this, I think this is not yours though.
Investigating.
Patch: http://gcc.gnu.org/ml/gcc-patches/2004-03/msg00672.html
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.
It appears that Richard/Jeff were either never asked or never approved this patch. Therefore, I've postponed it until GCC 3.4.1.
For the sake of completeness: they were both asked.
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
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.
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
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 :-)]
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
> 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).
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
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
Fixed, finally.
*** Bug 15512 has been marked as a duplicate of this bug. ***