[PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Jakub Jelinek]

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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Eric Botcazou]

> 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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Jakub Jelinek]

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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Eric Botcazou]

[-- 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.  */
 

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Jakub Jelinek]

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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Eric Botcazou]

> 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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Jakub Jelinek]

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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Eric Botcazou]

> 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

Re: [PATCH] Fix -fcompare-debug issue in reload1 (PR debug/46252)

[Eric Botcazou]

> 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