[PATCH] sel-sched: Fix erroneous re-use of pointer to last insn in a BB (PR 48235)

[PATCH] sel-sched: Fix erroneous re-use of pointer to last insn in a BB (PR 48235)

[Alexander Monakov]

Hello,

This patch fixes a couple of places where code motion machinery wrongly
attempts to re-use a pointer to the last insn in a BB after control flow
following that BB has been changed (so the last jump might have been removed
or replaced).  This is not too frequent, so the solution is to simply
recompute the last instruction if we notice the CFG change.

Bootstrapped and regtested on x86_64-linux together with other recently
submitted sel-sched fixes; OK for trunk?  (I forgot to mention this in two
other e-mails; sorry).


2011-04-07  Dmitry Melnik  <dm@ispras.ru>

	PR rtl-optimization/48235
	* sel-sched.c (code_motion_process_successors): Recompute the last
	insn in basic block if control flow changed.
	(code_motion_path_driver): Ditto.  Recompute the first insn as well.
	Update condition for ilist_remove.

testsuite:
	gcc.dg/pr48235.c: New.

diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 48fb2e0..f409c4f 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -6369,7 +6369,10 @@ code_motion_process_successors (insn_t insn, av_set_t orig_ops,
          the iterator becomes invalid.  We need to try again.  */
       if (BLOCK_FOR_INSN (insn)->index != old_index
           || EDGE_COUNT (bb->succs) != old_succs)
-        goto rescan;
+        {
+          insn = sel_bb_end (BLOCK_FOR_INSN (insn));
+          goto rescan;
+        }
     }
 
 #ifdef ENABLE_CHECKING
@@ -6587,21 +6590,37 @@ code_motion_path_driver (insn_t insn, av_set_t orig_ops, ilist_t path,
   if (!expr)
     {
       int res;
+      rtx last_insn = PREV_INSN (insn);
+      bool added_to_path;
 
       gcc_assert (insn == sel_bb_end (bb));
 
       /* Add bb tail to PATH (but it doesn't make any sense if it's a bb_head -
 	 it's already in PATH then).  */
       if (insn != first_insn)
-	ilist_add (&path, insn);
+	{
+	  ilist_add (&path, insn);
+	  added_to_path = true;
+	}
+      else
+        added_to_path = false;
 
       /* Process_successors should be able to find at least one
 	 successor for which code_motion_path_driver returns TRUE.  */
       res = code_motion_process_successors (insn, orig_ops,
                                             path, static_params);
 
+      /* Jump in the end of basic block could have been removed or replaced
+         during code_motion_process_successors, so recompute insn as the
+         last insn in bb.  */
+      if (NEXT_INSN (last_insn) != insn)
+        {
+          insn = sel_bb_end (bb);
+          first_insn = sel_bb_head (bb);
+        }
+
       /* Remove bb tail from path.  */
-      if (insn != first_insn)
+      if (added_to_path)
 	ilist_remove (&path);
 
       if (res != 1)
diff --git a/gcc/testsuite/gcc.dg/pr48235.c b/gcc/testsuite/gcc.dg/pr48235.c
new file mode 100644
index 0000000..8ec5edb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr48235.c
@@ -0,0 +1,57 @@
+/* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-options "-O -fno-guess-branch-probability -fpeel-loops -freorder-blocks-and-partition -fschedule-insns2 -fsel-sched-pipelining -fselective-scheduling2" } */
+struct intC
+{
+  short x;
+  short y;
+};
+
+int size_x;
+
+static inline int
+TileDiffXY (int x, int y)
+{
+  return (y * size_x) + x;
+}
+
+struct HangarTileTable
+{
+  struct intC ti;
+  int hangar_num;
+};
+
+struct AirportSpec
+{
+  struct HangarTileTable *depot_table;
+  int size;
+};
+
+void Get ();
+struct AirportSpec dummy;
+
+static inline int
+GetRotatedTileFromOffset (int *a, struct intC tidc)
+{
+  if (!*a)
+    Get ();
+  switch (*a)
+    {
+    case 0:
+      return (tidc.y << size_x) + tidc.x;
+    case 1:
+      return TileDiffXY (tidc.y, dummy.size - tidc.x);
+    case 2:
+      return TileDiffXY (tidc.x, dummy.size - tidc.y);
+    case 3:
+      return TileDiffXY (dummy.size - 1, tidc.x);
+    }
+}
+
+int
+GetHangarNum (int *a)
+{
+	int i;
+  for (i = 0; i < dummy.size; i++)
+    if (GetRotatedTileFromOffset (a, dummy.depot_table[i].ti))
+      return dummy.depot_table[i].hangar_num;
+}

Re: [PATCH] sel-sched: Fix erroneous re-use of pointer to last insn in a BB (PR 48235)

[Vladimir Makarov]

On 04/07/2011 01:45 PM, Alexander Monakov wrote:
> Hello,
>
> This patch fixes a couple of places where code motion machinery wrongly
> attempts to re-use a pointer to the last insn in a BB after control flow
> following that BB has been changed (so the last jump might have been removed
> or replaced).  This is not too frequent, so the solution is to simply
> recompute the last instruction if we notice the CFG change.
>
> Bootstrapped and regtested on x86_64-linux together with other recently
> submitted sel-sched fixes; OK for trunk?  (I forgot to mention this in two
> other e-mails; sorry).
>
>
> 2011-04-07  Dmitry Melnik<dm@ispras.ru>
>
> 	PR rtl-optimization/48235
> 	* sel-sched.c (code_motion_process_successors): Recompute the last
> 	insn in basic block if control flow changed.
> 	(code_motion_path_driver): Ditto.  Recompute the first insn as well.
> 	Update condition for ilist_remove.
>
> testsuite:
> 	gcc.dg/pr48235.c: New.
>
Ok, thanks.

Re: [PATCH] sel-sched: Fix erroneous re-use of pointer to last insn in a BB (PR 48235)

[Steve Ellcey]

Vladimir and Dmitry,

The new test you added, gcc.dg/pr48235.c, is failing on IA64 because
it gets this excess message:

cc1: note: -freorder-blocks-and-partition does not work on this architecture

Could you modify the test to either filter out this message or not 
use the -freorder-blocks-and-partition option in IA64 or not run the
test at all on IA64.  I notice that gcc.dg/tree-prof/pr45354.c looks
like this test but also has:

/* { dg-require-effective-target freorder } */

That test does not fail, or run, on IA64 due to this dg option.

Steve Ellcey
sje@cup.hp.com

Re: [PATCH] sel-sched: Fix erroneous re-use of pointer to last insn in a BB (PR 48235)

[Alexander Monakov]

On Fri, 15 Apr 2011, Steve Ellcey wrote:

> Vladimir and Dmitry,
> 
> The new test you added, gcc.dg/pr48235.c, is failing on IA64 because
> it gets this excess message:
> 
> cc1: note: -freorder-blocks-and-partition does not work on this architecture
> 
> Could you modify the test to either filter out this message or not 
> use the -freorder-blocks-and-partition option in IA64 or not run the
> test at all on IA64.  I notice that gcc.dg/tree-prof/pr45354.c looks
> like this test but also has:
> 
> /* { dg-require-effective-target freorder } */
> 
> That test does not fail, or run, on IA64 due to this dg option.

I have committed the following patch.  Sorry about the problem.

2011-04-18  Alexander Monakov  <amonakov@ispras.ru>

	* gcc.dg/pr48235.c: Add dg-require-effective-target freorder.

Index: gcc/testsuite/gcc.dg/pr48235.c
===================================================================
--- gcc/testsuite/gcc.dg/pr48235.c	(revision 172642)
+++ gcc/testsuite/gcc.dg/pr48235.c	(working copy)
@@ -1,4 +1,5 @@
 /* { dg-do compile { target powerpc*-*-* ia64-*-* i?86-*-* x86_64-*-* } } */
+/* { dg-require-effective-target freorder } */
 /* { dg-options "-O -fno-guess-branch-probability -fpeel-loops -freorder-blocks-and-partition -fschedule-insns2 -fsel-sched-pipelining -fselective-scheduling2" } */
 struct intC
 {