Bug 27883 - [4.0/4.1 regression] in schedule_insns, at sched-rgn.c:3038 on mips
Summary: [4.0/4.1 regression] in schedule_insns, at sched-rgn.c:3038 on mips
Status: RESOLVED WONTFIX
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.2.0
: P1 normal
Target Milestone: 4.1.2
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-checking, ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2006-06-03 14:42 UTC by Martin Michlmayr
Modified: 2007-02-10 15:17 UTC (History)
7 users (show)

See Also:
Host: mipsel-linux-gnu
Target: mipsel-linux-gnu
Build: mipsel-linux-gnu
Known to work: 3.4.0 4.2.0
Known to fail: 4.0.0 4.1.0 4.1.1 4.1.2
Last reconfirmed: 2006-07-22 22:07:25


Attachments
test case (637 bytes, text/plain)
2006-06-03 14:43 UTC, Martin Michlmayr
Details
Delete old REG_DEAD notes before creating new ones. (472 bytes, patch)
2006-07-11 01:41 UTC, Jim Wilson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Michlmayr 2006-06-03 14:42:47 UTC
I get the following ICE with current gcc 4.2 on mips.  It appeared some time between 20051124 and 20060419.  Unfortunately I don't have any compiler versions inbetween these dates around so I cannot narrow it down further.

(sid)tbm@swarm:~/delta/bin$ /usr/lib/gcc-snapshot/bin/g++ -c -O2 mini.c
mini.c: In member function 'void std::Radau15::Step(const std::Frame&, std::Frame&, std::Interaction*)':
mini.c:96: internal compiler error: in schedule_insns, at sched-rgn.c:3038
Please submit a full bug report,
with preprocessed source if appropriate.
See <URL:http://gcc.gnu.org/bugs.html> for instructions.
Comment 1 Martin Michlmayr 2006-06-03 14:43:55 UTC
Created attachment 11589 [details]
test case
Comment 2 Andrew Pinski 2006-06-03 16:01:46 UTC
I bet a beer that this was caused by:
2006-03-16  Maxim Kuvyrkov <mkuvyrkov@ispras.ru>

        * target.h (struct spec_info_def): New opaque declaration.
....
Comment 3 Maxim Kuvyrkov 2006-06-07 15:29:40 UTC
(In reply to comment #2)
> I bet a beer that this was caused by:
> 2006-03-16  Maxim Kuvyrkov <mkuvyrkov@ispras.ru>
> 
>         * target.h (struct spec_info_def): New opaque declaration.
> ....
> 

Nope.  This one is caused by combine.c .

1. Combine generates a REG_DEAD note of the register that don't even appear in the code.
2. This note survives life2 but gets deleted in sched-rgn.c: check_dead_notes1 ().
3. After scheduler is finished, it recalculates life info, not attaching a REG_DEAD note for that register (which is, I think, correct).
4. Scheduler fails on the assert.

So, the major problem is that combiner emits a strange REG_DEAD note (see combine.c: distribute_notes ()) and the minor problem is that life2 doesn't fix this (though, it would not generate such note by itself).
Comment 4 Jim Wilson 2006-06-27 03:02:40 UTC
This only fails for --enable-checking builds, which is the default for mainline, but not release branches.  I was able to reproduce this with gcc-4.0, but not gcc-3.4.  The difference between the two is that gcc-4.0 has the __builtin_copysign support, which emits the RTL that triggers the error.  The error has probably been latent for a while.

The insn that causes the problem is
(jump_insn 99 98 113 15 (set (pc)
        (if_then_else (ge:SI (subreg:SI (reg:DF 32 $f0) 4)
                (const_int 0 [0x0]))
            (label_ref 101)
            (pc))) 261 {*branch_zerosi} (insn_list:REG_DEP_TRUE 94 (nil))
    (expr_list:REG_DEAD (reg:SI 33 $f1)
        (expr_list:REG_BR_PROB (const_int 5000 [0x1388])
            (nil))))
On a 32-bit mips target, DFmode $f0 is a register pair $f0/$f1, so technically this is correct.  There is a corresponding REG_UNUSED for $f0 on the insn that sets (reg:DF 32 $f0).

However, flow doesn't track individual regs in subregs, it only tracks the whole subreg.  Note that mark_used_reg ignores subregs.  So when we execute the second life pass, we end up with
(jump_insn 98 97 112 17 (set (pc)
        (if_then_else (ge (subreg:SI (reg:DF 32 $f0) 4)
                (const_int 0 [0x0]))
            (label_ref 100)
            (pc))) 271 {*branch_ordersi} (insn_list:REG_DEP_TRUE 93 (nil))
    (expr_list:REG_DEAD (reg:DF 32 $f0)
        (expr_list:REG_DEAD (reg:SI 33 $f1)
            (expr_list:REG_BR_PROB (const_int 5000 [0x1388])
                (nil)))))
Note that we now have two overlapping REG_DEAD notes plus an overlapping REG_UNUSED note on a previous insn.  When sched runs, it deletes both REG_DEAD notes, but only readds one, resulting in the abort for a REG_DEAD note consistency problem.

A subreg of a hard register is normally not allowed, but it is created in this case because CANNOT_CHANGE_CLASS_MODE is defined, and HARD_REGNO_MODE_OK says that an SImode $f1 is not OK.  The result is that simplify_subreg doesn't simplify this.  The other part is that register_operand says it is OK.  Eventually, this gets fixed by reload.

Fixing combine to get this right looks complicated.  Combine has to know that the register was used inside a subreg, and then figure out that the subreg wasn't simplified because of CANNOT_CHANGE_CLASS_MODE, etc.

I think a simpler solution here is to note that the life2 pass would have worked correctly if it deleted all prior REG_UNUSED/REG_DEAD notes before it started.  Incidentally, the comments for the life2 pass say it was explicitly added to fix REG_UNUSED/REG_DEAD problems with distribute_notes in combine, so it was apparently added to fix a related problem.  It just isn't working the way it was originally intended.

It is curious that life2 is running immediately before sched, instead of immediately after combine.  If we ran it immediately after combine, we could get rid of the REG_DEAD/REG_UNUSED support in distribute_notes, which is probably the most complicated, and most buggy, part of combine.  This would also speed up the compiler a little bit.
Comment 5 Jim Wilson 2006-07-11 01:32:25 UTC
This is already fixed on dataflow-branch.  At the end of combine_instructions, it runs a global dataflow pass that removes all REG_DEAD and REG_UNUSED notes before creating new ones.
Comment 6 Jim Wilson 2006-07-11 01:41:55 UTC
Created attachment 11857 [details]
Delete old REG_DEAD notes before creating new ones.

This works for the testcase, but is otherwise untested.  This just deletes the old REG_DEAD notes for a local life update before creating new ones.  I believe this could be made simpler by eliminating UPDATE_LIFE_GLOBAL_RM_NOTES, and then calling count_or_remove_death_notes if PROP_DEATH_NOTES is set for both global and local updates.  However, assuming we want what is on dataflow-branch, it seems better to go with a safer non-conflicting patch for now, and let dataflow-branch be the long term fix.
Comment 7 Eric Christopher 2006-07-22 21:14:57 UTC
Jim, were you going to check this in or did you need some more testing on it?
Comment 8 Andrew Pinski 2006-07-22 22:07:25 UTC
This bug does not belong in waiting as it is not waiting on the user input but a developer input.
Comment 9 Jim Wilson 2006-07-24 19:34:43 UTC
Subject: Re:  [4.0/4.1/4.2 regression] in schedule_insns,
	at sched-rgn.c:3038 on mips

On Sat, 2006-07-22 at 14:14, echristo at apple dot com wrote:
> ------- Comment #7 from echristo at apple dot com  2006-07-22 21:14 -------
> Jim, were you going to check this in or did you need some more testing on it?

I haven't had time to test it yet.



Comment 10 Martin Michlmayr 2006-08-23 18:56:05 UTC
(In reply to comment #9)
> > Jim, were you going to check this in or did you need some more testing on it?
> 
> I haven't had time to test it yet.

Any update on this, Jim?
Comment 11 wilson@specifix.com 2006-08-23 21:25:00 UTC
Subject: Re:  [4.0/4.1/4.2 regression] in schedule_insns,
 at sched-rgn.c:3038 on mips

tbm at gcc dot gnu dot org wrote:
> Any update on this, Jim?

No.  My life continues to be busy and complicated, making it difficult 
to do much gcc work.

However, I did put a patch in the PR.  If anyone else has time, they 
could try testing it.  It needs a bootstrap and regression test.

I think the problem is best fixed on mainline by merging in the dataflow 
branch, but for currently supported releases, and for the pending 4.2 
release, I think the simple fix in the PR is best.  So if we add this 
fix to the mainline now, then we need to remember to remove it when the 
dataflow branch goes in, or else prevent it from migrating to the 
dataflow branch.
Comment 12 Martin Michlmayr 2006-08-25 19:05:50 UTC
Update: mzero6.c fails because of this ICE too.
Comment 13 Martin Michlmayr 2006-08-25 20:09:07 UTC
Reduced testcase:

double copysign (double x, double y);
double GetDouble();
double a = copysign (1.0, GetDouble());
Comment 14 Martin Michlmayr 2006-08-27 13:20:25 UTC
Bootstrapped and regtested successfully on mipsel-linux-gnu and x86_64-linux-gnu
Comment 15 Andrew Pinski 2006-09-07 04:24:34 UTC
Subject: Bug 27883

Author: pinskia
Date: Thu Sep  7 04:24:24 2006
New Revision: 116739

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=116739
Log:
2006-09-06  James E Wilson  <wilson@specifix.com>

        PR rtl-opt/27883
        * flow.c (update_life_info): If UPDATE_LIFE_LOCAL and PROP_DEATH_NOTES
        then call count_or_remove_death_notes.

2006-09-06  Andrew Pinski  <pinskia@physics.uc.edu>

        PR rtl-opt/27883
        * g++.dg/opt/copysign-1.C: New test.


Added:
    trunk/gcc/testsuite/g++.dg/opt/copysign-1.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/flow.c
    trunk/gcc/testsuite/ChangeLog

Comment 16 Andrew Pinski 2006-09-07 04:24:52 UTC
Fixed at least on the mainline now.
Comment 17 Jorn Wolfgang Rennecke 2006-10-31 14:34:49 UTC
(In reply to comment #4)
> It is curious that life2 is running immediately before sched, instead of
> immediately after combine.  If we ran it immediately after combine, we could
> get rid of the REG_DEAD/REG_UNUSED support in distribute_notes, which is
> probably the most complicated, and most buggy, part of combine.  This would
> also speed up the compiler a little bit.

The combiner itself relies on REG_DEAD notes to find out what it can combine.
If you leave an invalid REG_DEAD note, it can lead to wrong code being emitted.
If you don't leave in valid REG_DEAD notes in their proper place, multi-step
instruction combinations will stop working - and with the three insn limit on
any single combination, this is likely to stop quite a lot of combinations.
Most combiner bridge patterns will become useless.
Comment 18 Eric Botcazou 2007-01-24 11:30:46 UTC
Given that this is not user-visible in released compilers and won't very
likely lead to wrong code, I propose to mark it WONTFIX for 4.0 and 4.1.
Comment 19 Martin Michlmayr 2007-01-24 12:01:05 UTC
(In reply to comment #18)
> Given that this is not user-visible in released compilers and won't very
> likely lead to wrong code, I propose to mark it WONTFIX for 4.0 and 4.1.

Speaking as the submitter, I don't mind if this is done.

BTW, we should not forget Jim's Comment #11: the patch should be backed out from 4.3 when the dataflow branch is merged.  Which I believe is going to happen soon?
Comment 20 Eric Botcazou 2007-02-10 15:17:52 UTC
Not user-visible in 4.1.x.