[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 vries at gcc dot gnu.org changed: What|Removed |Added CC||dant at picochip dot com --- Comment #18 from vries at gcc dot gnu.org 2013-02-25 09:30:25 UTC --- Does that fix both this PR and the PR56242 regression? PR56242 happens when we're deleting an undeletable label (i.e. turning it into an deleted label note) and reordering that note in an insn stream with SEQUENCEs. This fix makes sure that the reordering is done only when we have a CFG. PR56242 has been reported for hppa. For that target, the CFG is not recomputed in pass_machine_reorg (For all targets, the CFG is freed before pass_machine_reorg). That means that the fix has the effect that PR56242 won't trigger any more on hppa. The more general question then is whether there are targets that both have sequences and have a CFG at the same time. I'll make an attempt at answering this question. For all these targets, we recompute the CFG at the start of pass_machine_reorg: ... $ egrep '(compute|free)_bb_for_insn' gcc/config/*/* gcc/config/arm/arm.c: compute_bb_for_insn (); gcc/config/bfin/bfin.c: compute_bb_for_insn (); gcc/config/c6x/c6x.c: compute_bb_for_insn (); gcc/config/frv/frv.c: compute_bb_for_insn (); gcc/config/i386/i386.c: compute_bb_for_insn (); gcc/config/ia64/ia64.c: compute_bb_for_insn (); gcc/config/mep/mep.c: compute_bb_for_insn (); gcc/config/mips/mips.c:compute_bb_for_insn (); gcc/config/mips/mips.c: free_bb_for_insn (); gcc/config/mn10300/mn10300.c: compute_bb_for_insn (); gcc/config/picochip/picochip.c: compute_bb_for_insn (); gcc/config/spu/spu.c: compute_bb_for_insn (); gcc/config/spu/spu.c: free_bb_for_insn (); gcc/config/spu/spu.c: compute_bb_for_insn (); gcc/config/spu/spu.c: free_bb_for_insn (); gcc/config/tilegx/tilegx.c: compute_bb_for_insn (); gcc/config/tilepro/tilepro.c: compute_bb_for_insn (); ... For mips, that is only briefly, before any SEQUENCE is created. For spu, it's during the whole of pass_machine_reorg, but not after. These targets can have sequences (disregarding frv because they're all in comments): ... $ egrep -c 'define_delay' gcc/config/*/* | egrep -v ':0|frv' gcc/config/cris/cris.md:3 gcc/config/fr30/fr30.md:1 gcc/config/h8300/h8300.md:1 gcc/config/iq2000/iq2000.md:3 gcc/config/microblaze/microblaze.md:1 gcc/config/mips/mips.md:4 gcc/config/pa/pa.md:7 gcc/config/picochip/picochip.md:1 gcc/config/sh/sh.md:4 gcc/config/sparc/sparc.md:5 ... Looking for targets that occur in both lists (and ignoring mips) I find only picochip. So, AFAIU, PR56242 might still trigger for picochip.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #19 from Jakub Jelinek jakub at gcc dot gnu.org 2013-02-25 10:32:36 UTC --- Please post the patch to gcc-patches, if there are issues on picochip, they can be resolved later, the patch will cure the issues on the primary and secondary architectures.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #20 from vries at gcc dot gnu.org 2013-02-25 11:50:30 UTC --- Author: vries Date: Mon Feb 25 11:50:25 2013 New Revision: 196255 URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=196255 Log: 2013-02-25 Tom de Vries t...@codesourcery.com PR rtl-optimization/56131 * insn-notes.def (INSN_NOTE_BASIC_BLOCK): Update comment. * cfgrtl.c (delete_insn): Don't reorder NOTE_INSN_DELETED_LABEL and NOTE_INSN_BASIC_BLOCK if BLOCK_FOR_INSN == NULL. Modified: trunk/gcc/ChangeLog trunk/gcc/cfgrtl.c trunk/gcc/insn-notes.def
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 vries at gcc dot gnu.org changed: What|Removed |Added Status|REOPENED|RESOLVED Resolution||FIXED --- Comment #21 from vries at gcc dot gnu.org 2013-02-25 11:51:18 UTC --- Fixed
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #22 from stevenb.gcc at gmail dot com stevenb.gcc at gmail dot com 2013-02-25 19:54:59 UTC --- Looking for targets that occur in both lists (and ignoring mips) I find only picochip. So, AFAIU, PR56242 might still trigger for picochip. No. It is impossible for a target to have SEQUENCEs and a CFG at the same time. The CFG code doesn't handle SEQUENCEs. Having SEQUENCEs for delay slots in the CFG would result in a verify_flow_info failure, because there would be non-jump insns (i.e. the SEQUENCE) at the end of basic blocks ending in a branch. Any target pretends to have a valid CFG and SEQUENCEs at the same time is broken.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #23 from stevenb.gcc at gmail dot com stevenb.gcc at gmail dot com 2013-02-25 19:59:09 UTC --- For all these targets, we recompute the CFG at the start of pass_machine_reorg: ... $ egrep '(compute|free)_bb_for_insn' gcc/config/*/* gcc/config/arm/arm.c: compute_bb_for_insn (); gcc/config/bfin/bfin.c: compute_bb_for_insn (); gcc/config/c6x/c6x.c: compute_bb_for_insn (); gcc/config/frv/frv.c: compute_bb_for_insn (); gcc/config/i386/i386.c: compute_bb_for_insn (); gcc/config/ia64/ia64.c: compute_bb_for_insn (); gcc/config/mep/mep.c: compute_bb_for_insn (); gcc/config/mips/mips.c:compute_bb_for_insn (); gcc/config/mips/mips.c: free_bb_for_insn (); gcc/config/mn10300/mn10300.c: compute_bb_for_insn (); gcc/config/picochip/picochip.c: compute_bb_for_insn (); gcc/config/spu/spu.c: compute_bb_for_insn (); gcc/config/spu/spu.c: free_bb_for_insn (); gcc/config/spu/spu.c: compute_bb_for_insn (); gcc/config/spu/spu.c: free_bb_for_insn (); gcc/config/tilegx/tilegx.c: compute_bb_for_insn (); gcc/config/tilepro/tilepro.c: compute_bb_for_insn (); For GCC 4.9 we should make freeing the CFG late the default. I have patches for that in my queue already.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 Eric Botcazou ebotcazou at gcc dot gnu.org changed: What|Removed |Added CC||ebotcazou at gcc dot ||gnu.org --- Comment #24 from Eric Botcazou ebotcazou at gcc dot gnu.org 2013-02-25 22:29:30 UTC --- No. It is impossible for a target to have SEQUENCEs and a CFG at the same time. The CFG code doesn't handle SEQUENCEs. Having SEQUENCEs for delay slots in the CFG would result in a verify_flow_info failure, because there would be non-jump insns (i.e. the SEQUENCE) at the end of basic blocks ending in a branch. FWIW that's my understanding as well. You need to shut down the CFG machinery once you start to introduce SEQUENCEs in the insn stream.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131
--- Comment #14 from vries at gcc dot gnu.org 2013-02-22 10:30:06 UTC ---
Steven,
thanks for the comments.
Nothing even trying to look at the CFG after freeing it, so the looks at
BLOCK_FOR_INSN in delete_insn are non-sense.
AFAIU now from
http://gcc.gnu.org/onlinedocs/gccint/Maintaining-the-CFG.html#Maintaining-the-CFG,
BLOCK_FOR_INSN == NULL shows whether the CFG has been freed or not, so I'd say
it makes sense to test for equality of BLOCK_FOR_INSN when non-NULL.
Looking for the basic block
anywhere at all at this point makes no sense,
If I understand you correctly with 'this point' you mean 'after the CFG has
been freed'?
basic block contents and
boundaries are not maintained and may be scrambled enough to make even
the basic block notes unreliable.
OK. I've added a comment in insn-notes.def in the patch below to make that more
explicit.
Also, If the label is not marked with a bb, assume it's the same bb
is wrong if the label is a marker for a constant pool or a jump table.
OK, fixed in patch below.
Does this patch address all your concerns?
...
Index: gcc/insn-notes.def
===
--- gcc/insn-notes.def (revision 195874)
+++ gcc/insn-notes.def (working copy)
@@ -70,7 +70,9 @@ INSN_NOTE (CALL_ARG_LOCATION)
/* Record the struct for the following basic block. Uses
NOTE_BASIC_BLOCK. FIXME: Redundant with the basic block pointer
- now included in every insn. */
+ now included in every insn. NOTE: If there's no CFG anymore, in other
words,
+ if BLOCK_FOR_INSN () == NULL, NOTE_BASIC_BLOCK cannot be considered
reliable
+ anymore. */
INSN_NOTE (BASIC_BLOCK)
/* Mark the inflection point in the instruction stream where we switch
Index: gcc/cfgrtl.c
===
--- gcc/cfgrtl.c (revision 195874)
+++ gcc/cfgrtl.c (working copy)
@@ -135,7 +135,7 @@ delete_insn (rtx insn)
if (! can_delete_label_p (insn))
{
const char *name = LABEL_NAME (insn);
- basic_block bb, label_bb = BLOCK_FOR_INSN (insn);
+ basic_block bb = BLOCK_FOR_INSN (insn);
rtx bb_note = NEXT_INSN (insn);
really_delete = false;
@@ -144,15 +144,13 @@ delete_insn (rtx insn)
NOTE_DELETED_LABEL_NAME (insn) = name;
/* If the note following the label starts a basic block, and the
- label is a member of the same basic block, interchange the two.
- If the label is not marked with a bb, assume it's the same bb. */
+ label is a member of the same basic block, interchange the two. */
if (bb_note != NULL_RTX
NOTE_INSN_BASIC_BLOCK_P (bb_note)
- (label_bb == NOTE_BASIC_BLOCK (bb_note)
- || label_bb == NULL))
+ bb != NULL
+ bb == BLOCK_FOR_INSN (bb_note))
{
reorder_insns_nobb (insn, insn, bb_note);
- bb = NOTE_BASIC_BLOCK (bb_note);
BB_HEAD (bb) = bb_note;
if (BB_END (bb) == bb_note)
BB_END (bb) = insn;
...
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 Jakub Jelinek jakub at gcc dot gnu.org changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #15 from Jakub Jelinek jakub at gcc dot gnu.org 2013-02-22 13:17:48 UTC --- Does that fix both this PR and the PR56242 regression? It looks reasonable to me.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #16 from Steven Bosscher steven at gcc dot gnu.org 2013-02-22 16:33:58 UTC --- (In reply to comment #14) Yes, iff the CFG hasn't been freed, looking at BLOCK_FOR_INSN is of course OK. I was referring to the situation after freeing the CFG. Adding that comment to insn-notes.def seems like a good idea. The patch looks reasonable. Does it fix bug 56242?
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #17 from vries at gcc dot gnu.org 2013-02-22 22:37:04 UTC --- patch below. Bootstrapped and reg-tested on x86_64 (ada inclusive).
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 Steven Bosscher steven at gcc dot gnu.org changed: What|Removed |Added Status|RESOLVED|REOPENED CC||steven at gcc dot gnu.org Resolution|FIXED | --- Comment #13 from Steven Bosscher steven at gcc dot gnu.org 2013-02-21 10:29:25 UTC --- The fix for this PR is wrong. Nothing even trying to look at the CFG after freeing it, so the looks at BLOCK_FOR_INSN in delete_insn are non-sense. Looking for the basic block anywhere at all at this point makes no sense, basic block contents and boundaries are not maintained and may be scrambled enough to make even the basic block notes unreliable. Also, If the label is not marked with a bb, assume it's the same bb is wrong if the label is a marker for a constant pool or a jump table.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #11 from vries at gcc dot gnu.org 2013-02-06 08:53:41 UTC --- Author: vries Date: Wed Feb 6 08:53:32 2013 New Revision: 195784 URL: http://gcc.gnu.org/viewcvs?root=gccview=revrev=195784 Log: 2013-02-06 Tom de Vries t...@codesourcery.com PR rtl-optimization/56131 * cfgrtl.c (delete_insn): Use NOTE_BASIC_BLOCK instead of BLOCK_FOR_INSN to get the bb of a NOTE_INSN_BASIC_BLOCK. Handle the case that the bb of the label is NULL. Add comment. Modified: trunk/gcc/ChangeLog trunk/gcc/cfgrtl.c
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 vries at gcc dot gnu.org changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution||FIXED --- Comment #12 from vries at gcc dot gnu.org 2013-02-06 12:24:05 UTC --- patch checked in, ICE triggered on existing test-case to no need for extra test-case. Marking resolved-fixed.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 Rainer Orth ro at gcc dot gnu.org changed: What|Removed |Added Target|sparc-linux |sparc-*-* CC||ro at gcc dot gnu.org --- Comment #7 from Rainer Orth ro at gcc dot gnu.org 2013-02-04 13:08:35 UTC --- Same on 32-bit Solaris/SPARC only, 64-bit is fine. Rainer
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 vries at gcc dot gnu.org changed: What|Removed |Added Status|NEW |ASSIGNED --- Comment #8 from vries at gcc dot gnu.org 2013-02-04 15:18:50 UTC --- Mikael, I tested this on x86_64-linux and sparc64-linux. On x86_64 there were no test suite changes, Thanks for testing this patch. Was the run on x86_64 a bootstrap-and-test? If not, I'll start one now. Thanks, - Tom
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #9 from Mikael Pettersson mikpe at it dot uu.se 2013-02-04 15:39:09 UTC --- (In reply to comment #8) Mikael, I tested this on x86_64-linux and sparc64-linux. On x86_64 there were no test suite changes, Thanks for testing this patch. Was the run on x86_64 a bootstrap-and-test? If not, I'll start one now. Both of them were full bootstrap-and-test runs.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 vries at gcc dot gnu.org changed: What|Removed |Added Keywords||ice-on-valid-code, patch --- Comment #10 from vries at gcc dot gnu.org 2013-02-04 17:35:13 UTC --- submitted patch: http://gcc.gnu.org/ml/gcc-patches/2013-02/msg00122.html
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 --- Comment #6 from Mikael Pettersson mikpe at it dot uu.se 2013-01-30 22:20:19 UTC --- (In reply to comment #5) A more structured version of the patch: After fixing the obvious syntax error + If the label is not marked with a bb, assume it's the same bb. */ + */ I tested this on x86_64-linux and sparc64-linux. On x86_64 there were no test suite changes, on sparc64 I got -FAIL: gcc.dg/pr56035.c (internal compiler error) -FAIL: gcc.dg/pr56035.c (test for excess errors) which is good, but I also got +FAIL: g++.old-deja/g++.pt/memtemp52.C -std=c++11 (test for excess errors) g++.log shows it failed due to an exit 1 from xg++, without diagnostic. Re-running that one test manually doesn't fail so I guess it was just a fluke.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 Richard Biener rguenth at gcc dot gnu.org changed: What|Removed |Added Component|regression |rtl-optimization Target Milestone|--- |4.8.0
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131 John David Anglin danglin at gcc dot gnu.org changed: What|Removed |Added CC||danglin at gcc dot gnu.org --- Comment #4 from John David Anglin danglin at gcc dot gnu.org 2013-01-29 17:01:20 UTC --- Also seen on hppa64-hp-hpux11.11.
[Bug rtl-optimization/56131] [4.8 regression] gcc.dg/pr56035.c ICEs gcc on sparc-linux
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56131
--- Comment #5 from vries at gcc dot gnu.org 2013-01-29 19:34:08 UTC ---
A more structured version of the patch:
...
Index: cfgrtl.c
===
--- cfgrtl.c (revision 195240)
+++ cfgrtl.c (working copy)
@@ -135,7 +135,7 @@ delete_insn (rtx insn)
if (! can_delete_label_p (insn))
{
const char *name = LABEL_NAME (insn);
- basic_block bb = BLOCK_FOR_INSN (insn);
+ basic_block bb = NULL, label_bb = BLOCK_FOR_INSN (insn);
rtx bb_note = NEXT_INSN (insn);
really_delete = false;
@@ -143,8 +143,17 @@ delete_insn (rtx insn)
NOTE_KIND (insn) = NOTE_INSN_DELETED_LABEL;
NOTE_DELETED_LABEL_NAME (insn) = name;
- if (bb_note != NULL_RTX NOTE_INSN_BASIC_BLOCK_P (bb_note)
- BLOCK_FOR_INSN (bb_note) == bb)
+ if (bb_note != NULL_RTX
+ NOTE_INSN_BASIC_BLOCK_P (bb_note))
+bb = NOTE_BASIC_BLOCK (bb_note);
+
+ /* If the note following the label starts a basic block, and the
+ label is a member of the same basic block, interchange the two.
+ If the label is not marked with a bb, assume it's the same bb. */
+ */
+ if (bb != NULL
+ (bb == label_bb
+ || label_bb == NULL))
{
reorder_insns_nobb (insn, insn, bb_note);
BB_HEAD (bb) = bb_note;
...
I'll try to bootstrap this patch on x86_64 coming weekend when I'll be back
home. I've also asked for access to the gcc compile farm to be able to test
this on a sparc machine.
