Patchwork [4.7?] Fix PR 52250

login
register
mail settings
Submitter Andrey Belevantsev
Date March 5, 2012, 8:47 a.m.
Message ID <4F547DA5.5000202@ispras.ru>
Download mbox | patch
Permalink /patch/144629/
State New
Headers show

Comments

Andrey Belevantsev - March 5, 2012, 8:47 a.m.
Hello,

The PR is about selective scheduling not trying hard enough to find a 
neighbor block to stick the note list from an empty block being removed. 
Fixed by a) trying harder and b) making sure that if we fail to find the 
right place, we just drop the notes instead of asserting.

Tested on x86-64 and ia64, approved by Alexander offline.  No test as it 
has quite insane number of flags that would likely stop reproducing the 
core issue in a few weeks.

Richard, Jakub, I can commit this to trunk today or to wait until the 
release.  The patch is very safe and so can be committed in case we will 
need an RC2, if you want one bug less in a release, but I would feel 
equally happy to postpone it until 4.7.1 as the problematic situation is 
quite rare (and probably requires pipelining outer loops to trigger which 
is disabled with ia64/-O3).

Yours,
Andrey

2012-03-05  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/52250
	* sel-sched-ir.c (maybe_tidy_empty_bb): Try harder to find a bb
	to put note list into.  Unconditionally call move_bb_info.
	(move_bb_info): Do not assert the blocks being in the same region,
	just drop the note list if they are not.
Richard Guenther - March 5, 2012, 9:45 a.m.
2012/3/5 Andrey Belevantsev <abel@ispras.ru>:
> Hello,
>
> The PR is about selective scheduling not trying hard enough to find a
> neighbor block to stick the note list from an empty block being removed.
> Fixed by a) trying harder and b) making sure that if we fail to find the
> right place, we just drop the notes instead of asserting.
>
> Tested on x86-64 and ia64, approved by Alexander offline.  No test as it has
> quite insane number of flags that would likely stop reproducing the core
> issue in a few weeks.
>
> Richard, Jakub, I can commit this to trunk today or to wait until the
> release.  The patch is very safe and so can be committed in case we will
> need an RC2, if you want one bug less in a release, but I would feel equally
> happy to postpone it until 4.7.1 as the problematic situation is quite rare
> (and probably requires pipelining outer loops to trigger which is disabled
> with ia64/-O3).

Note that 4.7 has branched, so you can commit to trunk.  As it's a
non-standard option please defer backport until after the release of 4.7.0.

Thanks,
Richard.

> Yours,
> Andrey
>
> 2012-03-05  Andrey Belevantsev  <abel@ispras.ru>
>
>        PR rtl-optimization/52250
>        * sel-sched-ir.c (maybe_tidy_empty_bb): Try harder to find a bb
>        to put note list into.  Unconditionally call move_bb_info.
>        (move_bb_info): Do not assert the blocks being in the same region,
>        just drop the note list if they are not.
>
>
> diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
> index a93cd68..c53d2e1 100644
> --- a/gcc/sel-sched-ir.c
> +++ b/gcc/sel-sched-ir.c
> @@ -3658,7 +3658,7 @@ sel_recompute_toporder (void)
>  static bool
>  maybe_tidy_empty_bb (basic_block bb)
>  {
> -  basic_block succ_bb, pred_bb;
> +  basic_block succ_bb, pred_bb, note_bb;
>   VEC (basic_block, heap) *dom_bbs;
>   edge e;
>   edge_iterator ei;
> @@ -3697,6 +3697,17 @@ maybe_tidy_empty_bb (basic_block bb)
>   pred_bb = NULL;
>   dom_bbs = NULL;
>
> +  /* Save a pred/succ from the current region to attach the notes to.  */
> +  note_bb = NULL;
> +  FOR_EACH_EDGE (e, ei, bb->preds)
> +    if (in_current_region_p (e->src))
> +      {
> +       note_bb = e->src;
> +       break;
> +      }
> +  if (note_bb == NULL)
> +    note_bb = succ_bb;
> +
>   /* Redirect all non-fallthru edges to the next bb.  */
>   while (rescan_p)
>     {
> @@ -3746,10 +3757,8 @@ maybe_tidy_empty_bb (basic_block bb)
>   else
>     {
>       /* This is a block without fallthru predecessor.  Just delete it.  */
> -      gcc_assert (pred_bb != NULL);
> -
> -      if (in_current_region_p (pred_bb))
> -       move_bb_info (pred_bb, bb);
> +      gcc_assert (note_bb);
> +      move_bb_info (note_bb, bb);
>       remove_empty_bb (bb, true);
>     }
>
> @@ -5231,10 +5240,9 @@ sel_remove_bb (basic_block bb, bool
> remove_from_cfg_p)
>  static void
>  move_bb_info (basic_block merge_bb, basic_block empty_bb)
>  {
> -  gcc_assert (in_current_region_p (merge_bb));
> -
> -  concat_note_lists (BB_NOTE_LIST (empty_bb),
> -                    &BB_NOTE_LIST (merge_bb));
> +  if (in_current_region_p (merge_bb))
> +    concat_note_lists (BB_NOTE_LIST (empty_bb),
> +                      &BB_NOTE_LIST (merge_bb));
>   BB_NOTE_LIST (empty_bb) = NULL_RTX;
>
>  }

Patch

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index a93cd68..c53d2e1 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -3658,7 +3658,7 @@  sel_recompute_toporder (void)
  static bool
  maybe_tidy_empty_bb (basic_block bb)
  {
-  basic_block succ_bb, pred_bb;
+  basic_block succ_bb, pred_bb, note_bb;
    VEC (basic_block, heap) *dom_bbs;
    edge e;
    edge_iterator ei;
@@ -3697,6 +3697,17 @@  maybe_tidy_empty_bb (basic_block bb)
    pred_bb = NULL;
    dom_bbs = NULL;

+  /* Save a pred/succ from the current region to attach the notes to.  */
+  note_bb = NULL;
+  FOR_EACH_EDGE (e, ei, bb->preds)
+    if (in_current_region_p (e->src))
+      {
+       note_bb = e->src;
+       break;
+      }
+  if (note_bb == NULL)
+    note_bb = succ_bb;
+
    /* Redirect all non-fallthru edges to the next bb.  */
    while (rescan_p)
      {
@@ -3746,10 +3757,8 @@  maybe_tidy_empty_bb (basic_block bb)
    else
      {
        /* This is a block without fallthru predecessor.  Just delete it.  */
-      gcc_assert (pred_bb != NULL);
-
-      if (in_current_region_p (pred_bb))
-       move_bb_info (pred_bb, bb);
+      gcc_assert (note_bb);
+      move_bb_info (note_bb, bb);
        remove_empty_bb (bb, true);
      }

@@ -5231,10 +5240,9 @@  sel_remove_bb (basic_block bb, bool remove_from_cfg_p)
  static void
  move_bb_info (basic_block merge_bb, basic_block empty_bb)
  {
-  gcc_assert (in_current_region_p (merge_bb));
-
-  concat_note_lists (BB_NOTE_LIST (empty_bb),
-                    &BB_NOTE_LIST (merge_bb));
+  if (in_current_region_p (merge_bb))
+    concat_note_lists (BB_NOTE_LIST (empty_bb),
+                      &BB_NOTE_LIST (merge_bb));
    BB_NOTE_LIST (empty_bb) = NULL_RTX;

  }