[PATCH] Fix PR 44919

[PATCH] Fix PR 44919

[Andrey Belevantsev]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

Hello,

As noted in the PR, when we have added support for moving insns through 
mutually exclusive insns, it was made possible to move conditional jumps 
across block boundaries.  But we didn't fix move_cond_jump accordingly, as 
it assumes that the jump can be moved only through insns in the same block.
The fix is to support moving a jump through series of fallthru blocks which 
all have only mutually exclusive insns with a jump.  So to do this, the 
failing assert was changed to an enable-checking code that ensures the 
several blocks case only happens with mutually exclusive insns, that the 
actual code to do movement is rewritten via reorder_insns instead of manual 
twiddling with PREV/NEXT_INSN and df_insn_change_bb.

Now, the ICE can be seen only on 4.4 branch, on trunk the problematic code 
is not generated, so I wonder how we got in 4.4 a series of fallthru basic 
blocks in first place, they seem to exist before the scheduler is run. 
Anyways, even for the branch the fix seems to be quite safe.

Bootstrap and test on ia64 is in progress. OK for trunk if it succeeds? 
4.4?  The patch misses adding a test case from PR, this will be done of course.

Thanks, Andrey

2010-08-24  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/44919

	* sel-sched.c (move_cond_jump): Remove assert, check that
	the several blocks case can only happen with mutually exclusive
	insns instead.  Rewrite the movement code to support moving through
	several basic blocks.

[-- Attachment #2: pr44919-context --]
[-- Type: text/plain, Size: 4264 bytes --]

Index: gcc/sel-sched.c
===================================================================
*** gcc/sel-sched.c	(revision 163516)
--- gcc/sel-sched.c	(working copy)
*************** static void
*** 4875,4892 ****
  move_cond_jump (rtx insn, bnd_t bnd)
  {
    edge ft_edge;
!   basic_block block_from, block_next, block_new;
!   rtx next, prev, link;
  
-   /* BLOCK_FROM holds basic block of the jump.  */
    block_from = BLOCK_FOR_INSN (insn);
  
!   /* Moving of jump should not cross any other jumps or
!   beginnings of new basic blocks.  */
!   gcc_assert (block_from == BLOCK_FOR_INSN (BND_TO (bnd)));
  
    /* Jump is moved to the boundary.  */
-   prev = BND_TO (bnd);
    next = PREV_INSN (insn);
    BND_TO (bnd) = insn;
  
--- 4875,4909 ----
  move_cond_jump (rtx insn, bnd_t bnd)
  {
    edge ft_edge;
!   basic_block block_from, block_next, block_new, block_bnd, bb;
!   rtx next, prev, link, head;
  
    block_from = BLOCK_FOR_INSN (insn);
+   block_bnd = BLOCK_FOR_INSN (BND_TO (bnd));
+   prev = BND_TO (bnd);
  
! #ifdef ENABLE_CHECKING
!   /* Moving of jump should not cross any other jumps or beginnings of new
!      basic blocks.  The only exception is when we move a jump through
!      mutually exclusive insns along fallthru edges.  */
!   if (block_from != block_bnd)
!     {
!       bb = block_from;
!       for (link = PREV_INSN (insn); link != PREV_INSN (prev);
!            link = PREV_INSN (link))
!         {
!           if (INSN_P (link))
!             gcc_assert (sched_insns_conditions_mutex_p (insn, link));
!           if (BLOCK_FOR_INSN (link) && BLOCK_FOR_INSN (link) != bb)
!             {
!               gcc_assert (single_pred (bb) == BLOCK_FOR_INSN (link));
!               bb = BLOCK_FOR_INSN (link);
!             }
!         }
!     }
! #endif
  
    /* Jump is moved to the boundary.  */
    next = PREV_INSN (insn);
    BND_TO (bnd) = insn;
  
*************** move_cond_jump (rtx insn, bnd_t bnd)
*** 4901,4928 ****
    gcc_assert (block_new->next_bb == block_next
                && block_from->next_bb == block_new);
  
!   gcc_assert (BB_END (block_from) == insn);
! 
!   /* Move all instructions except INSN from BLOCK_FROM to
!      BLOCK_NEW.  */
!   for (link = prev; link != insn; link = NEXT_INSN (link))
      {
!       EXPR_ORIG_BB_INDEX (INSN_EXPR (link)) = block_new->index;
!       df_insn_change_bb (link, block_new);
!     }
  
!   /* Set correct basic block and instructions properties.  */
!   BB_END (block_new) = PREV_INSN (insn);
  
!   NEXT_INSN (PREV_INSN (prev)) = insn;
!   PREV_INSN (insn) = PREV_INSN (prev);
  
    /* Assert there is no jump to BLOCK_NEW, only fallthrough edge.  */
    gcc_assert (NOTE_INSN_BASIC_BLOCK_P (BB_HEAD (block_new)));
-   PREV_INSN (prev) = BB_HEAD (block_new);
-   NEXT_INSN (next) = NEXT_INSN (BB_HEAD (block_new));
-   NEXT_INSN (BB_HEAD (block_new)) = prev;
-   PREV_INSN (NEXT_INSN (next)) = next;
  
    gcc_assert (!sel_bb_empty_p (block_from)
                && !sel_bb_empty_p (block_new));
--- 4918,4952 ----
    gcc_assert (block_new->next_bb == block_next
                && block_from->next_bb == block_new);
  
!   /* Move all instructions except INSN to BLOCK_NEW.  */
!   bb = block_bnd;
!   head = BB_HEAD (block_new);
!   while (bb != block_from->next_bb)
      {
!       rtx from, to;
!       from = bb == block_bnd ? prev : sel_bb_head (bb);
!       to = bb == block_from ? next : sel_bb_end (bb);
  
!       /* The jump being moved can be the first insn in the block.
!          In this case we don't have to move anything in this block.  */
!       if (NEXT_INSN (to) != from)
!         {
!           reorder_insns (from, to, head);
  
!           for (link = to; link != head; link = PREV_INSN (link))
!             EXPR_ORIG_BB_INDEX (INSN_EXPR (link)) = block_new->index;
!           head = to;
!         }
! 
!       /* Cleanup possibly empty blocks left.  */
!       block_next = bb->next_bb;
!       if (bb != block_from)
!           maybe_tidy_empty_bb (bb);
!       bb = block_next;
!     }
  
    /* Assert there is no jump to BLOCK_NEW, only fallthrough edge.  */
    gcc_assert (NOTE_INSN_BASIC_BLOCK_P (BB_HEAD (block_new)));
  
    gcc_assert (!sel_bb_empty_p (block_from)
                && !sel_bb_empty_p (block_new));

Re: [PATCH] Fix PR 44919

[Vladimir Makarov]

  On 08/24/2010 06:06 AM, Andrey Belevantsev wrote:
> Hello,
>
> As noted in the PR, when we have added support for moving insns 
> through mutually exclusive insns, it was made possible to move 
> conditional jumps across block boundaries.  But we didn't fix 
> move_cond_jump accordingly, as it assumes that the jump can be moved 
> only through insns in the same block.
> The fix is to support moving a jump through series of fallthru blocks 
> which all have only mutually exclusive insns with a jump.  So to do 
> this, the failing assert was changed to an enable-checking code that 
> ensures the several blocks case only happens with mutually exclusive 
> insns, that the actual code to do movement is rewritten via 
> reorder_insns instead of manual twiddling with PREV/NEXT_INSN and 
> df_insn_change_bb.
>
> Now, the ICE can be seen only on 4.4 branch, on trunk the problematic 
> code is not generated, so I wonder how we got in 4.4 a series of 
> fallthru basic blocks in first place, they seem to exist before the 
> scheduler is run. Anyways, even for the branch the fix seems to be 
> quite safe.
>
> Bootstrap and test on ia64 is in progress. OK for trunk if it 
> succeeds? 4.4?  The patch misses adding a test case from PR, this will 
> be done of course.
>
> Thanks, Andrey
>
> 2010-08-24  Andrey Belevantsev <abel@ispras.ru>
>
>     PR rtl-optimization/44919
>
>     * sel-sched.c (move_cond_jump): Remove assert, check that
>     the several blocks case can only happen with mutually exclusive
>     insns instead.  Rewrite the movement code to support moving through
>     several basic blocks.
Ok for the trunk and 4.4 branch.  Thanks for working on the PR.

Re: [PATCH] Fix PR 44919

[Paolo Carlini]

... as far as I can see, this patch is breaking the build:

libbackend.a(sel-sched.o): In function `move_cond_jump':
/home/paolo/Gcc/svn-dirs/trunk/gcc/sel-sched.c:4944: undefined reference
to `maybe_tidy_empty_bb'
collect2: ld returned 1 exit status
make[2]: *** [lto1] Error 1
make[2]: *** Waiting for unfinished jobs....
libbackend.a(sel-sched.o): In function `move_cond_jump':
/home/paolo/Gcc/svn-dirs/trunk/gcc/sel-sched.c:4944: undefined reference
to `maybe_tidy_empty_bb'
collect2: ld returned 1 exit status
make[2]: *** [cc1-dummy] Error 1
libbackend.a(sel-sched.o): In function `move_cond_jump':
/home/paolo/Gcc/svn-dirs/trunk/gcc/sel-sched.c:4944: undefined reference
to `maybe_tidy_empty_bb'
collect2: ld returned 1 exit status
make[2]: *** [cc1plus-dummy] Error 1

Paolo.

Re: [PATCH] Fix PR 44919

[Alexander Monakov]

On Mon, 6 Sep 2010, Paolo Carlini wrote:

> ... as far as I can see, this patch is breaking the build:

Yes, I checked in a 4.4 patch into trunk by mistake.  I have committed the
following patch to fix the build error.

2010-09-06  Alexander Monakov  <amonakov@ispras.ru>

        * sel-sched.c (move_cond_jump): Correct arguments to maybe_tidy_empty_bb.
        * sel-sched-ir.c (maybe_tidy_empty_bb): Export.

Index: sel-sched.c
===================================================================
--- sel-sched.c	(revision 163904)
+++ sel-sched.c	(working copy)
@@ -4941,7 +4941,7 @@
       /* Cleanup possibly empty blocks left.  */
       block_next = bb->next_bb;
       if (bb != block_from)
-          maybe_tidy_empty_bb (bb);
+	maybe_tidy_empty_bb (bb, false);
       bb = block_next;
     }
 
Index: sel-sched-ir.c
===================================================================
--- sel-sched-ir.c	(revision 163903)
+++ sel-sched-ir.c	(working copy)
@@ -3540,7 +3540,7 @@
 }
 
 /* Tidy the possibly empty block BB.  */
-static bool
+bool
 maybe_tidy_empty_bb (basic_block bb, bool recompute_toporder_p)
 {
   basic_block succ_bb, pred_bb;
Index: sel-sched-ir.h
===================================================================
--- sel-sched-ir.h	(revision 163903)
+++ sel-sched-ir.h	(working copy)
@@ -1619,6 +1619,7 @@
 extern void free_bb_note_pool (void);
 
 extern void sel_remove_empty_bb (basic_block, bool, bool);
+extern bool maybe_tidy_empty_bb (basic_block, bool);
 extern void purge_empty_blocks (void);
 extern basic_block sel_split_edge (edge);
 extern basic_block sel_create_recovery_block (insn_t);