* [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
@ 2010-11-01 19:31 Jakub Jelinek
2010-11-02 18:46 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2010-11-01 19:31 UTC (permalink / raw)
To: gcc-patches
Hi!
delete_dead_insn removes unused preceeding insn, but will not do that
if there are debug insns in between.
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
2010-11-01 Jakub Jelinek <jakub@redhat.com>
PR debug/46252
* rtl.h (prev_real_nondebug_insn): New prototype.
* emit-rtl.c (prev_real_nondebug_insn): New function.
* reload1.c (delete_dead_insn): Use it instead of
prev_real_insn.
* cfgcleanup.c (try_head_merge_bb): Likewise.
* gcc.dg/pr46252.c: New test.
--- gcc/rtl.h.jj 2010-11-01 09:07:23.000000000 +0100
+++ gcc/rtl.h 2010-11-01 15:41:31.000000000 +0100
@@ -1751,6 +1751,7 @@ extern rtx prev_nonnote_nondebug_insn (r
extern rtx next_nonnote_nondebug_insn (rtx);
extern rtx prev_real_insn (rtx);
extern rtx next_real_insn (rtx);
+extern rtx prev_real_nondebug_insn (rtx);
extern rtx prev_active_insn (rtx);
extern rtx next_active_insn (rtx);
extern int active_insn_p (const_rtx);
--- gcc/emit-rtl.c.jj 2010-11-01 09:07:23.000000000 +0100
+++ gcc/emit-rtl.c 2010-11-01 15:41:01.000000000 +0100
@@ -3133,8 +3133,8 @@ next_real_insn (rtx insn)
return insn;
}
-/* Return the last INSN, CALL_INSN or JUMP_INSN before INSN;
- or 0, if there is none. This routine does not look inside
+/* Return the last INSN, CALL_INSN, JUMP_INSN or DEBUG_INSN
+ before INSN; or 0, if there is none. This routine does not look inside
SEQUENCEs. */
rtx
@@ -3150,6 +3150,23 @@ prev_real_insn (rtx insn)
return insn;
}
+/* Return the last INSN, CALL_INSN or JUMP_INSN before INSN;
+ or 0, if there is none. This routine does not look inside
+ SEQUENCEs. */
+
+rtx
+prev_real_nondebug_insn (rtx insn)
+{
+ while (insn)
+ {
+ insn = PREV_INSN (insn);
+ if (insn == 0 || NONDEBUG_INSN_P (insn))
+ break;
+ }
+
+ return insn;
+}
+
/* Return the last CALL_INSN in the current list, or 0 if there is none.
This routine does not look inside SEQUENCEs. */
--- gcc/reload1.c.jj 2010-11-01 09:07:24.000000000 +0100
+++ gcc/reload1.c 2010-11-01 15:39:21.000000000 +0100
@@ -2112,7 +2112,7 @@ spill_failure (rtx insn, enum reg_class
static void
delete_dead_insn (rtx insn)
{
- rtx prev = prev_real_insn (insn);
+ rtx prev = prev_real_nondebug_insn (insn);
rtx prev_dest;
/* If the previous insn sets a register that dies in our insn, delete it
--- gcc/cfgcleanup.c.jj 2010-11-01 09:07:23.000000000 +0100
+++ gcc/cfgcleanup.c 2010-11-01 15:42:08.000000000 +0100
@@ -2054,9 +2054,7 @@ try_head_merge_bb (basic_block bb)
max_match--;
if (max_match == 0)
return false;
- do
- e0_last_head = prev_real_insn (e0_last_head);
- while (DEBUG_INSN_P (e0_last_head));
+ e0_last_head = prev_real_nondebug_insn (e0_last_head);
}
if (max_match == 0)
--- gcc/testsuite/gcc.dg/pr46252.c.jj 2010-11-01 16:12:22.000000000 +0100
+++ gcc/testsuite/gcc.dg/pr46252.c 2010-11-01 16:12:07.000000000 +0100
@@ -0,0 +1,15 @@
+/* PR debug/46252 */
+/* { dg-do compile } */
+/* { dg-options "-O -frerun-cse-after-loop -fno-tree-loop-optimize -funroll-loops -fcompare-debug" } */
+
+void
+foo (float *f)
+{
+ int i;
+ for (i = 0; i < 4; i++)
+ f[i] = i;
+ bar ();
+ for (i = 0; i < 4; i++)
+ if (f[i] != i)
+ __builtin_abort ();
+}
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-01 19:31 [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252) Jakub Jelinek
@ 2010-11-02 18:46 ` Eric Botcazou
2010-11-02 18:58 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2010-11-02 18:46 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Alexandre Oliva
> delete_dead_insn removes unused preceeding insn, but will not do that
> if there are debug insns in between.
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
>
> 2010-11-01 Jakub Jelinek <jakub@redhat.com>
>
> PR debug/46252
> * rtl.h (prev_real_nondebug_insn): New prototype.
> * emit-rtl.c (prev_real_nondebug_insn): New function.
> * reload1.c (delete_dead_insn): Use it instead of
> prev_real_insn.
> * cfgcleanup.c (try_head_merge_bb): Likewise.
The question is, shouldn't prev_real_insn (and next_real_insn) avoid returning
a DEBUG_INSN in the first place? The remaining use of prev_real_insn seems
to want prev_real_nondebug_insn as well.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-02 18:46 ` Eric Botcazou
@ 2010-11-02 18:58 ` Jakub Jelinek
2010-11-02 19:51 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2010-11-02 18:58 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Alexandre Oliva
On Tue, Nov 02, 2010 at 07:42:43PM +0100, Eric Botcazou wrote:
> > delete_dead_insn removes unused preceeding insn, but will not do that
> > if there are debug insns in between.
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk?
> >
> > 2010-11-01 Jakub Jelinek <jakub@redhat.com>
> >
> > PR debug/46252
> > * rtl.h (prev_real_nondebug_insn): New prototype.
> > * emit-rtl.c (prev_real_nondebug_insn): New function.
> > * reload1.c (delete_dead_insn): Use it instead of
> > prev_real_insn.
> > * cfgcleanup.c (try_head_merge_bb): Likewise.
>
> The question is, shouldn't prev_real_insn (and next_real_insn) avoid returning
> a DEBUG_INSN in the first place? The remaining use of prev_real_insn seems
> to want prev_real_nondebug_insn as well.
For prev_real_insn I think you're right, the remaining two uses also do want
to skip debug insns. For next_real_insn I'm not that sure, it is used e.g.
in the scheduler where we want to see debug insns.
Having prev_real_insn skip debug insns and next_real_insn not skip them
would be inconsistent though.
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-02 18:58 ` Jakub Jelinek
@ 2010-11-02 19:51 ` Eric Botcazou
2010-11-02 20:09 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2010-11-02 19:51 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Alexandre Oliva
[-- Attachment #1: Type: text/plain, Size: 690 bytes --]
> For prev_real_insn I think you're right, the remaining two uses also do
> want to skip debug insns.
It's really only one use, insert_insn_end_bb_new is unused, removed by the
attached patch.
> For next_real_insn I'm not that sure, it is used e.g. in the scheduler where
> we want to see debug insns.
> Having prev_real_insn skip debug insns and next_real_insn not skip them
> would be inconsistent though.
Should we replace all the calls to prev_real_insn by prev_active_insn then and
delete the former? The *_active_insn family doesn't return DEBUG_INSNS...
* basic-block.h (insert_insn_end_bb_new): Delete.
* cfgrtl.c (insert_insn_end_bb_new): Likewise.
--
Eric Botcazou
[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 4092 bytes --]
Index: basic-block.h
===================================================================
--- basic-block.h (revision 166172)
+++ basic-block.h (working copy)
@@ -881,9 +881,6 @@ extern basic_block get_bb_copy (basic_bl
void set_loop_copy (struct loop *, struct loop *);
struct loop *get_loop_copy (struct loop *);
-
-extern rtx insert_insn_end_bb_new (rtx, basic_block);
-
#include "cfghooks.h"
/* Return true when one of the predecessor edges of BB is marked with EDGE_EH. */
Index: cfgrtl.c
===================================================================
--- cfgrtl.c (revision 166172)
+++ cfgrtl.c (working copy)
@@ -3090,94 +3090,6 @@ init_rtl_bb_info (basic_block bb)
bb->il.rtl = ggc_alloc_cleared_rtl_bb_info ();
}
-
-/* Add EXPR to the end of basic block BB. */
-
-rtx
-insert_insn_end_bb_new (rtx pat, basic_block bb)
-{
- rtx insn = BB_END (bb);
- rtx new_insn;
- rtx pat_end = pat;
-
- while (NEXT_INSN (pat_end) != NULL_RTX)
- pat_end = NEXT_INSN (pat_end);
-
- /* If the last insn is a jump, insert EXPR in front [taking care to
- handle cc0, etc. properly]. Similarly we need to care trapping
- instructions in presence of non-call exceptions. */
-
- if (JUMP_P (insn)
- || (NONJUMP_INSN_P (insn)
- && (!single_succ_p (bb)
- || single_succ_edge (bb)->flags & EDGE_ABNORMAL)))
- {
-#ifdef HAVE_cc0
- rtx note;
-#endif
- /* If this is a jump table, then we can't insert stuff here. Since
- we know the previous real insn must be the tablejump, we insert
- the new instruction just before the tablejump. */
- if (GET_CODE (PATTERN (insn)) == ADDR_VEC
- || GET_CODE (PATTERN (insn)) == ADDR_DIFF_VEC)
- insn = prev_real_insn (insn);
-
-#ifdef HAVE_cc0
- /* FIXME: 'twould be nice to call prev_cc0_setter here but it aborts
- if cc0 isn't set. */
- note = find_reg_note (insn, REG_CC_SETTER, NULL_RTX);
- if (note)
- insn = XEXP (note, 0);
- else
- {
- rtx maybe_cc0_setter = prev_nonnote_insn (insn);
- if (maybe_cc0_setter
- && INSN_P (maybe_cc0_setter)
- && sets_cc0_p (PATTERN (maybe_cc0_setter)))
- insn = maybe_cc0_setter;
- }
-#endif
- /* FIXME: What if something in cc0/jump uses value set in new
- insn? */
- new_insn = emit_insn_before_noloc (pat, insn, bb);
- }
-
- /* Likewise if the last insn is a call, as will happen in the presence
- of exception handling. */
- else if (CALL_P (insn)
- && (!single_succ_p (bb)
- || single_succ_edge (bb)->flags & EDGE_ABNORMAL))
- {
- /* Keeping in mind targets with small register classes and parameters
- in registers, we search backward and place the instructions before
- the first parameter is loaded. Do this for everyone for consistency
- and a presumption that we'll get better code elsewhere as well. */
-
- /* Since different machines initialize their parameter registers
- in different orders, assume nothing. Collect the set of all
- parameter registers. */
- insn = find_first_parameter_load (insn, BB_HEAD (bb));
-
- /* If we found all the parameter loads, then we want to insert
- before the first parameter load.
-
- If we did not find all the parameter loads, then we might have
- stopped on the head of the block, which could be a CODE_LABEL.
- If we inserted before the CODE_LABEL, then we would be putting
- the insn in the wrong basic block. In that case, put the insn
- after the CODE_LABEL. Also, respect NOTE_INSN_BASIC_BLOCK. */
- while (LABEL_P (insn)
- || NOTE_INSN_BASIC_BLOCK_P (insn))
- insn = NEXT_INSN (insn);
-
- new_insn = emit_insn_before_noloc (pat, insn, bb);
- }
- else
- new_insn = emit_insn_after_noloc (pat, insn, bb);
-
- return new_insn;
-}
-
/* Returns true if it is possible to remove edge E by redirecting
it to the destination of the other edge from E->src. */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-02 19:51 ` Eric Botcazou
@ 2010-11-02 20:09 ` Jakub Jelinek
2010-11-02 20:48 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2010-11-02 20:09 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Alexandre Oliva
On Tue, Nov 02, 2010 at 08:31:55PM +0100, Eric Botcazou wrote:
> > For prev_real_insn I think you're right, the remaining two uses also do
> > want to skip debug insns.
>
> It's really only one use, insert_insn_end_bb_new is unused, removed by the
> attached patch.
>
> > For next_real_insn I'm not that sure, it is used e.g. in the scheduler where
> > we want to see debug insns.
> > Having prev_real_insn skip debug insns and next_real_insn not skip them
> > would be inconsistent though.
>
> Should we replace all the calls to prev_real_insn by prev_active_insn then and
> delete the former? The *_active_insn family doesn't return DEBUG_INSNS...
Yes, but after reload it also skips USE/CLOBBERs. I'm not sure we want to
do that in cfgcleanup, in reload1.c it should be fine, as reload_completed
there is still 0, and in gcse.c it is fine too.
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-02 20:09 ` Jakub Jelinek
@ 2010-11-02 20:48 ` Eric Botcazou
2010-11-03 0:03 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2010-11-02 20:48 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Alexandre Oliva
> Yes, but after reload it also skips USE/CLOBBERs. I'm not sure we want to
> do that in cfgcleanup, in reload1.c it should be fine, as reload_completed
> there is still 0, and in gcse.c it is fine too.
There is another flavor of *_active_insn in ifcvt.c. :-) Would it work to:
- export last_active_insn from ifcvt.c,
- call it in cfgcleanup.c,
- replace the other uses of prev_real_insn by prev_active_insn,
- delete prev_real_insn,
- document that next_real_insn can return DEBUG_INSNs (and that its uses may
need to be audited wrt DEBUG_INSNs in a ??? note)?
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-02 20:48 ` Eric Botcazou
@ 2010-11-03 0:03 ` Jakub Jelinek
2010-11-03 8:32 ` Eric Botcazou
2010-11-04 16:16 ` Eric Botcazou
0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2010-11-03 0:03 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Alexandre Oliva
On Tue, Nov 02, 2010 at 09:46:54PM +0100, Eric Botcazou wrote:
> > Yes, but after reload it also skips USE/CLOBBERs. I'm not sure we want to
> > do that in cfgcleanup, in reload1.c it should be fine, as reload_completed
> > there is still 0, and in gcse.c it is fine too.
>
> There is another flavor of *_active_insn in ifcvt.c. :-) Would it work to:
> - export last_active_insn from ifcvt.c,
> - call it in cfgcleanup.c,
> - replace the other uses of prev_real_insn by prev_active_insn,
> - delete prev_real_insn,
> - document that next_real_insn can return DEBUG_INSNs (and that its uses may
> need to be audited wrt DEBUG_INSNs in a ??? note)?
So far I have done 3) from the above list, bootstrapped/regtested on
x86_64-linux and i686-linux, getting tired so I've postponed the rest for
later on, will try to get to it soon. Ok?
2010-11-01 Jakub Jelinek <jakub@redhat.com>
PR debug/46252
* reload1.c (delete_dead_insn): Use prev_active_insn instead of
prev_real_insn.
* gcse.c (insert_insn_end_basic_block): Likewise.
* gcc.dg/pr46252.c: New test.
--- gcc/reload1.c.jj 2010-11-01 09:07:24.000000000 +0100
+++ gcc/reload1.c 2010-11-01 15:39:21.000000000 +0100
@@ -2112,7 +2112,7 @@ spill_failure (rtx insn, enum reg_class
static void
delete_dead_insn (rtx insn)
{
- rtx prev = prev_real_insn (insn);
+ rtx prev = prev_active_insn (insn);
rtx prev_dest;
/* If the previous insn sets a register that dies in our insn, delete it
--- gcc/gcse.c.jj 2010-09-13 20:54:20.000000000 +0200
+++ gcc/gcse.c 2010-11-02 22:37:14.022260983 +0100
@@ -3574,7 +3574,7 @@ insert_insn_end_basic_block (struct expr
the new instruction just before the tablejump. */
if (GET_CODE (PATTERN (insn)) == ADDR_VEC
|| GET_CODE (PATTERN (insn)) == ADDR_DIFF_VEC)
- insn = prev_real_insn (insn);
+ insn = prev_active_insn (insn);
#ifdef HAVE_cc0
/* FIXME: 'twould be nice to call prev_cc0_setter here but it aborts
--- gcc/testsuite/gcc.dg/pr46252.c.jj 2010-11-01 16:12:22.000000000 +0100
+++ gcc/testsuite/gcc.dg/pr46252.c 2010-11-01 16:12:07.000000000 +0100
@@ -0,0 +1,15 @@
+/* PR debug/46252 */
+/* { dg-do compile } */
+/* { dg-options "-O -frerun-cse-after-loop -fno-tree-loop-optimize -funroll-loops -fcompare-debug" } */
+
+void
+foo (float *f)
+{
+ int i;
+ for (i = 0; i < 4; i++)
+ f[i] = i;
+ bar ();
+ for (i = 0; i < 4; i++)
+ if (f[i] != i)
+ __builtin_abort ();
+}
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-03 0:03 ` Jakub Jelinek
@ 2010-11-03 8:32 ` Eric Botcazou
2010-11-04 16:16 ` Eric Botcazou
1 sibling, 0 replies; 9+ messages in thread
From: Eric Botcazou @ 2010-11-03 8:32 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Alexandre Oliva
> 2010-11-01 Jakub Jelinek <jakub@redhat.com>
>
> PR debug/46252
> * reload1.c (delete_dead_insn): Use prev_active_insn instead of
> prev_real_insn.
> * gcse.c (insert_insn_end_basic_block): Likewise.
>
> * gcc.dg/pr46252.c: New test.
OK, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)
2010-11-03 0:03 ` Jakub Jelinek
2010-11-03 8:32 ` Eric Botcazou
@ 2010-11-04 16:16 ` Eric Botcazou
1 sibling, 0 replies; 9+ messages in thread
From: Eric Botcazou @ 2010-11-04 16:16 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Alexandre Oliva
> So far I have done 3) from the above list, bootstrapped/regtested on
> x86_64-linux and i686-linux, getting tired so I've postponed the rest for
> later on, will try to get to it soon.
While the 3rd use of prev_real_insn in the middle-end has disappeared in the
meantime, it turns out that it is used in several back-ends.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-11-04 16:01 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-01 19:31 [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252) Jakub Jelinek
2010-11-02 18:46 ` Eric Botcazou
2010-11-02 18:58 ` Jakub Jelinek
2010-11-02 19:51 ` Eric Botcazou
2010-11-02 20:09 ` Jakub Jelinek
2010-11-02 20:48 ` Eric Botcazou
2010-11-03 0:03 ` Jakub Jelinek
2010-11-03 8:32 ` Eric Botcazou
2010-11-04 16:16 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).