Bug 15296 - [3.4 only] Delayed branch scheduling causing invalid code on cris-*
[3.4 only] Delayed branch scheduling causing invalid code on cris-*
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: rtl-optimization
3.2.1
: P1 critical
: 3.4.1
Assigned To: Hans-Peter Nilsson
: patch, wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-05-05 14:08 UTC by Hans-Peter Nilsson
Modified: 2004-06-13 02:00 UTC (History)
3 users (show)

See Also:
Host:
Target: cris-*
Build:
Known to work:
Known to fail: 3.4.0 3.3.3
Last reconfirmed: 2004-05-05 14:09:16


Attachments
executable test-case (1.50 KB, text/plain)
2004-05-05 14:12 UTC, Hans-Peter Nilsson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hans-Peter Nilsson 2004-05-05 14:08:19 UTC
reorg.c messes up register liveness info by deleting its own
placeholder insns.  Further analysis with forthcoming patch.
First seen in 3.2.x.  On 3.3-branch, 3.4-branch the problem
is exposed by the to-be-attached test-case but it's hidden
on trunk.
Comment 1 Hans-Peter Nilsson 2004-05-05 14:12:58 UTC
Created attachment 6226 [details]
executable test-case

Will be committed as gcc.c-torture/execute/pr15296.c
Comment 2 Hans-Peter Nilsson 2004-05-05 16:05:13 UTC
<URL:http://gcc.gnu.org/ml/gcc-patches/2004-05/msg00246.html>
Comment 3 CVS Commits 2004-05-07 03:20:28 UTC
Subject: Bug 15296

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	hp@gcc.gnu.org	2004-05-07 03:20:25

Modified files:
	gcc            : ChangeLog reorg.c 

Log message:
	PR optimization/15296
	* reorg.c (fill_simple_delay_slots): Use next_real_insn when
	getting last consecutive label at a branch.
	(relax_delay_slots): Similar, near top of loop.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.3593&r2=2.3594
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reorg.c.diff?cvsroot=gcc&r1=1.93&r2=1.94

Comment 4 CVS Commits 2004-05-07 03:22:38 UTC
Subject: Bug 15296

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	hp@gcc.gnu.org	2004-05-07 03:22:34

Modified files:
	gcc/testsuite  : ChangeLog 

Log message:
	PR optimization/15296
	* gcc.c-torture/execute/pr15296.c: New test.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3723&r2=1.3724

Comment 5 Hans-Peter Nilsson 2004-05-07 03:24:52 UTC
Committed on trunk, awaiting resolution for 3.3 and 3.4 branches.
Comment 6 Gabriel Dos Reis 2004-05-16 22:14:40 UTC
(In reply to comment #2)
> <URL:http://gcc.gnu.org/ml/gcc-patches/2004-05/msg00246.html>

This is OK for gcc-3_3-branch.
Comment 7 CVS Commits 2004-05-26 13:19:32 UTC
Subject: Bug 15296

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_3-branch
Changes by:	hp@gcc.gnu.org	2004-05-26 13:19:22

Modified files:
	gcc            : ChangeLog reorg.c 

Log message:
	PR optimization/15296
	* reorg.c (fill_simple_delay_slots): Use next_real_insn when
	getting last consecutive label at a branch.
	(relax_delay_slots): Similar, near top of loop.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.16114.2.984&r2=1.16114.2.985
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reorg.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.82.4.1&r2=1.82.4.2

Comment 8 CVS Commits 2004-05-26 13:24:17 UTC
Subject: Bug 15296

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_3-branch
Changes by:	hp@gcc.gnu.org	2004-05-26 13:24:07

Modified files:
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.c-torture/execute: pr15296.c 

Log message:
	PR optimization/15296
	* gcc.c-torture/execute/pr15296.c: New test.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.2261.2.375&r2=1.2261.2.376
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/pr15296.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=NONE&r2=1.1.10.1

Comment 9 Hans-Peter Nilsson 2004-05-26 17:08:45 UTC
Subject: Re:  [3.4 only] Delayed branch scheduling
 causing invalid code on cris-*

On Wed, 26 May 2004, pinskia at gcc dot gnu dot org wrote:
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>       Known to fail|3.3.3 3.4.0                 |3.4.0
>       Known to work|3.5.0                       |3.5.0 3.3.3

No, 3.3.3 is still known to fail.

brgds, H-P
Comment 10 Hans-Peter Nilsson 2004-05-26 17:11:21 UTC
I also removed 3.5.0 from "known to work" since it isn't released: we
can't "know" anything about it.  Please don't change anything.
Comment 11 Andrew Pinski 2004-05-26 17:16:09 UTC
Maybe I misunderstand the use of known to fail, since I thought your patch fixed it in 3.3.4 since you 
applied it there, likewise for 3.5.0 as that is what the known to fail is for.  Yes I did mess up by 
including 3.3.3 that should have been 3.3.4 woops but you can test 3.5.0 but you said it was fixed by 
your patch already, there might be a different bug which effects the now merged tree-ssa but that 
should be a different bug as none of the tree-ssa merge touched this code.
Comment 12 Hans-Peter Nilsson 2004-05-26 18:04:16 UTC
I really think so.  How can you *know* that the test-case
succeeds in 3.5.0 when until it's released?  These "known to"
fields are an indication from a user, not a statement from a
developer: that's what "target milestone" is for.  With your
interpretation one of "known to" and "target milestone" fields
would be redundant, and there would not be a field to specify
what *released* versions are known to work/fail.  The "reported 
against" is just the first spotting.  Now please *do not* change
it again without proper references.

If you think my interpretation is incorrect, do take it up on
gcc@ (please CC: me, I'm behind on the list reading) and/or  
point me to the proper documentation.  I can't find anything
specific for the "known to" fields at
<URL:http://gcc.gnu.org/management.html> or
<URL:http://www.bugzilla.org/docs/html/> (but target
milestone is documented).
Comment 13 Wolfgang Bangerth 2004-05-26 18:21:09 UTC
All of us bugzilla people quite frequently use the known-to-{work,fail} 
fields to fill which _branches_ work. For example, if something is 
known to fail with the 3.4.0 release but to be fixed on mainline, then 
3.5.0 will be listed in the known-to-work field. This actually makes some 
sense, since if it is fixed on the mainline, then there needs to be a 
testcase in the testsuite which (at least ideally) should prevent this 
problem from re-appearing on mainline, thus guaranteeing that the actual 
3.5.0 release will also work. 
 
This procedure may not be documented, but us bugzilla guys all seem to use 
it. 
 
W. 
Comment 14 Hans-Peter Nilsson 2004-05-26 18:40:38 UTC
Subject: Re:  [3.4 only] Delayed branch scheduling
 causing invalid code on cris-*

On Wed, 26 May 2004, bangerth at dealii dot org wrote:
> All of us bugzilla people quite frequently use the known-to-{work,fail}
> fields to fill which _branches_ work. For example, if something is
> known to fail with the 3.4.0 release but to be fixed on mainline, then
> 3.5.0 will be listed in the known-to-work field. This actually makes some
> sense, since if it is fixed on the mainline, then there needs to be a
> testcase in the testsuite which (at least ideally) should prevent this
> problem from re-appearing on mainline, thus guaranteeing that the actual
> 3.5.0 release will also work.

You may think it makes sense, but it is still confusing:
consider the bug re-appearing; perhaps because the fix was
reverted.  Then later, someone looks at bugzilla to choose a
"safe" version regarding the bug.  That person would be fooled
by the assertion that a bug is fixed in a version where the
testcase provingly fails.  Hence "known to work/fail" should
only be used for released versions. That is, unless extra work
is done at the release (or for any change that could possibly
affect the "known to" facts), checking the "known to work"
fields, adjusting them when necessary.

Besides, how do you compare the "known to work" field with the
"target version" field?  What about the apparent redundancy?

>
> This procedure may not be documented, but us bugzilla guys all seem to use
> it.

Which doesn't make it correct or any less confusing.

Anyway, if you stand by that usage, please document it for the
benefit of everyone else.  I'd like to be able to explain the
person reporting the bug to me (or anyone, for example a
manager), without refering to folklore.  1/2 :-)

brgds, H-P
Comment 15 Hans-Peter Nilsson 2004-05-26 18:43:20 UTC
Subject: Re:  [3.4 only] Delayed branch scheduling
 causing invalid code on cris-*

On Wed, 26 May 2004, Hans-Peter Nilsson wrote:
> Besides, how do you compare the "known to work" field with the
> "target version" field?  What about the apparent redundancy?

Sorry, should've been "target milestone".

brgds, H-P

Comment 16 Wolfgang Bangerth 2004-05-26 19:09:12 UTC
If a patch is reverted, the PR must be reopened anyway, and the person 
doing so should ideally also adjust the fields that this changes. Note 
that bugs should really not re-appear in other circumstances, since patches 
are supposed to come with testcases. 
 
The target milestone is always the first upcoming version on the first 
active branch on which the bug appears, unless the release manager for 
that branch decides that this bug can't be fixed for this release. Note 
that the target milestone describes an _intent_: "we'd like to have this 
bug fixed by that version". The known-to-work field describes a _fact_: 
"This bug has been tested against these versions/branches and has been 
verified to trigger/to not trigger the bug there". 
 
Thus, the known-to-* fields are not meant to indicate something to a 
user, but rather to fellow developers, namely where we have checked the 
bug and for example whether a patch has to be applied to the 3.4 or 3.3 
branch as well since the bug exists there as well. If we have verified that 
the bug doesn't exist there, that means that the developer does not have 
to spend cycles on re-checking this. 
 
I understand that the fact that we also use branch-names and not only 
release names in these fields can in a few occasions be confusing.  
However, I cannot imagine a significant number of cases where us setting 
the known-to-work field for branch x.y will be wrong for release x.y 
because the bug has reappeared. I believe that these should be very 
infrequent cases, and that we are much better served if we use this 
field to indicate where people have tested against a certain bug. 
 
W. 
Comment 17 Hans-Peter Nilsson 2004-05-26 21:02:12 UTC
Subject: Re:  [3.4 only] Delayed branch scheduling
 causing invalid code on cris-*

On Wed, 26 May 2004, bangerth at dealii dot org wrote:
> Note
> that the target milestone describes an _intent_: "we'd like to have this
> bug fixed by that version". The known-to-work field describes a _fact_:
> "This bug has been tested against these versions/branches and has been
> verified to trigger/to not trigger the bug there".

Well, that's what I've been trying to say...  The known-to
fields should state facts, but your current usage does not
follow the intent you state: when you write release identifiers
rather than branch identifiers, it can't be a *fact* until the
release is done.

Can you consider e.g. writing 3.3-branch, not 3.3.4 in the
known-to-work field?  It doesn't have to be an exact branch
identifier as long as it's unique.

Perhaps also consider using the known-to-fail field for releases
only?  If not, you'd have to iterate over all bugs to fix
known-to-fail fields for bugs that "disappear" for that field to
state a fact!

> I understand that the fact that we also use branch-names and not only
> release names in these fields can in a few occasions be confusing.

The problem seems to be that you use non-unique terms in those
fields: your refer to the 3.3 branch as 3.3.4, which will
hopefully be the name of a release as well.

Still, please document whatever usage of yours, supposedly in
management.html.  Maybe I should open a bug report for that?

(No, *I* don't want to document it since I still consider this
usage confusing if not wrong, if nothing else than that it's
easy to mistakenly remove a "known to fail" field for a release
known to fail (QED).  At least until you stop using the same
name for releases as branches.)

brgds, H-P
Comment 18 Wolfgang Bangerth 2004-05-26 21:52:37 UTC
I understand your reasoning, though I fail to see where it will 
actually bite us in practice. The fact that we don't document could 
be improved, though. Please open a a PR for this, and either copy 
the contents of our discussion there, or put a link to this PR. I 
myself will now have to leave for a nice Memorial Day weekend :) 
 
W. 
Comment 19 Hans-Peter Nilsson 2004-05-26 22:54:09 UTC
> I fail to see where it will actually bite us in practice.

Lack of imagination? :-) Consider the time put into this issue:
enough evidence of a problem, methinks. 8-)

> Please open a a PR for this
bug 15669 (gotta love those generated links)
Comment 20 Giovanni Bajo 2004-06-06 03:56:45 UTC
Mark, there is a proposed patch for this which already went into mainline and 
3.3 as far as I can see. Is it OK for 3.4 as well?
Comment 21 Mark Mitchell 2004-06-07 14:24:32 UTC
This patch is OK for 3.4.1; please apply it.
Comment 22 Mark Mitchell 2004-06-12 22:16:12 UTC
See Comment #20.
Comment 23 CVS Commits 2004-06-13 01:59:06 UTC
Subject: Bug 15296

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	giovannibajo@gcc.gnu.org	2004-06-13 01:58:58

Modified files:
	gcc            : ChangeLog reorg.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.c-torture/execute: pr15296.c 

Log message:
	PR rtl-optimization/15296
	* reorg.c (fill_simple_delay_slots): Use next_real_insn when
	getting last consecutive label at a branch.
	(relax_delay_slots): Similar, near top of loop.
	
	PR rtl-optimization/15296
	* gcc.c-torture/execute/pr15296.c: New test.

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.498&r2=2.2326.2.499
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reorg.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.90.4.1&r2=1.90.4.2
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.205&r2=1.3389.2.206
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/pr15296.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.18.1

Comment 24 Giovanni Bajo 2004-06-13 02:00:10 UTC
Fixed by applying HP's patch to 3.4 branch.