[PATCH] sel-sched: Avoid placing bookkeeping code above a fence (PR49349)

[PATCH] sel-sched: Avoid placing bookkeeping code above a fence (PR49349)

[Alexander Monakov]

Hello,

Quoting myself from the PR audit trail,

It's a rare bug in sel-sched: we fail to schedule some code in non-pipelining
mode.  The root cause is that we put bookkeeping instructions above a fence
that is placed on the last insn (uncond. jump) of the bookkeeping block.  We
could either make such blocks ineligible for bookkeeping or rewind such fences
from the jump back to the bookkeeping code (there's also a more involved
approach of re-introducing the idea of using local nops as placeholders for
fences).  I'm testing the following patch that implements the second approach
(as it should result in a bit cleaner code in such situations).

I'm also removing a conditional that allows NULL place_to_insert in
generate_bookkeeping_insn, as I don't see how it can possibly happen with
current implementation of find_place_for_bookkeeping.

Bootstrapped and regtested on ia64-linux, OK for trunk?  Steve Ellcey
confirmed that HP-UX testing is OK as well.

2011-06-14  Alexander Monakov  <amonakov@ispras.ru>

	PR target/49349
	* sel-sched.c (find_place_for_bookkeeping): Add new parameter
	(fence_to_rewind).  Use it to notice when bookkeeping will be placed
	above a fence.  Update comments.
	(generate_bookkeeping_insn): Rewind fence when bookkeeping code is
	placed just above it.  Do not allow NULL place_to_insert.

diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 3f22a3c..92ba222 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -4663,9 +4663,10 @@ create_block_for_bookkeeping (edge e1, edge e2)
 }
 
 /* Return insn after which we must insert bookkeeping code for path(s) incoming
-   into E2->dest, except from E1->src.  */
+   into E2->dest, except from E1->src.  If the returned insn immediately
+   precedes a fence, assign that fence to *FENCE_TO_REWIND.  */
 static insn_t
-find_place_for_bookkeeping (edge e1, edge e2)
+find_place_for_bookkeeping (edge e1, edge e2, fence_t *fence_to_rewind)
 {
   insn_t place_to_insert;
   /* Find a basic block that can hold bookkeeping.  If it can be found, do not
@@ -4707,9 +4708,14 @@ find_place_for_bookkeeping (edge e1, edge e2)
 	sel_print ("Pre-existing bookkeeping block is %i\n", book_block->index);
     }
 
-  /* If basic block ends with a jump, insert bookkeeping code right before it.  */
+  *fence_to_rewind = NULL;
+  /* If basic block ends with a jump, insert bookkeeping code right before it.
+     Notice if we are crossing a fence when taking PREV_INSN.  */
   if (INSN_P (place_to_insert) && control_flow_insn_p (place_to_insert))
-    place_to_insert = PREV_INSN (place_to_insert);
+    {
+      *fence_to_rewind = flist_lookup (fences, place_to_insert);
+      place_to_insert = PREV_INSN (place_to_insert);
+    }
 
   return place_to_insert;
 }
@@ -4784,21 +4790,23 @@ generate_bookkeeping_insn (expr_t c_expr, edge e1, edge e2)
   insn_t join_point, place_to_insert, new_insn;
   int new_seqno;
   bool need_to_exchange_data_sets;
+  fence_t fence_to_rewind;
 
   if (sched_verbose >= 4)
     sel_print ("Generating bookkeeping insn (%d->%d)\n", e1->src->index,
 	       e2->dest->index);
 
   join_point = sel_bb_head (e2->dest);
-  place_to_insert = find_place_for_bookkeeping (e1, e2);
-  if (!place_to_insert)
-    return NULL;
+  place_to_insert = find_place_for_bookkeeping (e1, e2, &fence_to_rewind);
   new_seqno = find_seqno_for_bookkeeping (place_to_insert, join_point);
   need_to_exchange_data_sets
     = sel_bb_empty_p (BLOCK_FOR_INSN (place_to_insert));
 
   new_insn = emit_bookkeeping_insn (place_to_insert, c_expr, new_seqno);
 
+  if (fence_to_rewind)
+    FENCE_INSN (fence_to_rewind) = new_insn;
+
   /* When inserting bookkeeping insn in new block, av sets should be
      following: old basic block (that now holds bookkeeping) data sets are
      the same as was before generation of bookkeeping, and new basic block

Re: [PATCH] sel-sched: Avoid placing bookkeeping code above a fence (PR49349)

[Vladimir Makarov]

On 06/14/2011 07:34 AM, Alexander Monakov wrote:
> Hello,
>
> Quoting myself from the PR audit trail,
>
> It's a rare bug in sel-sched: we fail to schedule some code in non-pipelining
> mode.  The root cause is that we put bookkeeping instructions above a fence
> that is placed on the last insn (uncond. jump) of the bookkeeping block.  We
> could either make such blocks ineligible for bookkeeping or rewind such fences
> from the jump back to the bookkeeping code (there's also a more involved
> approach of re-introducing the idea of using local nops as placeholders for
> fences).  I'm testing the following patch that implements the second approach
> (as it should result in a bit cleaner code in such situations).
>
> I'm also removing a conditional that allows NULL place_to_insert in
> generate_bookkeeping_insn, as I don't see how it can possibly happen with
> current implementation of find_place_for_bookkeeping.
>
> Bootstrapped and regtested on ia64-linux, OK for trunk?  Steve Ellcey
> confirmed that HP-UX testing is OK as well.
>
Ok.  Thanks, Alexander.
> 2011-06-14  Alexander Monakov<amonakov@ispras.ru>
>
> 	PR target/49349
> 	* sel-sched.c (find_place_for_bookkeeping): Add new parameter
> 	(fence_to_rewind).  Use it to notice when bookkeeping will be placed
> 	above a fence.  Update comments.
> 	(generate_bookkeeping_insn): Rewind fence when bookkeeping code is
> 	placed just above it.  Do not allow NULL place_to_insert.
>