Re: [Patch, bfin/c6x] Fix ICE for backends that rely on reorder_loops.

[Felix Yang <fei dot yang0953 at gmail dot com>]

Ping x1

Any suggestions on this?

The background is that we cannot call cfg_layout_initialize after the
bb-reorder pass
as indicated by the following code snippet:

  /* Once bb reordering is complete, cfg layout mode should not be re-entered.
     Entering cfg layout mode will perform optimizations on the cfg that
     could affect the bb layout negatively or even require fixups. An
     example of the latter is if edge forwarding performed when optimizing
     the cfg layout required moving a block from the hot to the cold section
     under -freorder-blocks-and-partition. This would create an illegal
     partitioning unless some manual fixup was performed.  */
  gcc_assert (!crtl->bb_reorder_complete);

I just removed the reorder_loops functionality and will consider
implementing a similiar feature
at the end of the bb-reorder pass as a next step.

Cheers,
Felix


On Wed, Jan 1, 2014 at 9:01 PM, Felix Yang <fei.yang0953@gmail.com> wrote:
> cc1 backtrace:
>
> arraysum.c: In function 'test_entry':
> arraysum.c:14:1: internal compiler error: in cfg_layout_initialize, at
> cfgrtl.c:4233
>  }
>  ^
> 0x6c096d cfg_layout_initialize(unsigned int)
>     ../../trunk/gcc/cfgrtl.c:4233
> 0xeab763 reorder_loops
>     ../../trunk/gcc/hw-doloop.c:500
> 0xeacd04 reorg_loops(bool, hw_doloop_hooks*)
>     ../../trunk/gcc/hw-doloop.c:641
> 0xdb8d2e c6x_hwloops
>     ../../trunk/gcc/config/c6x/c6x.c:5898
> 0xdbdb10 c6x_reorg
>     ../../trunk/gcc/config/c6x/c6x.c:5939
> 0xa61e30 rest_of_handle_machine_reorg
>     ../../trunk/gcc/reorg.c:3921
> 0xa61e5c execute
>     ../../trunk/gcc/reorg.c:3951
>
> Attached please find the patch for this ICE.
> Since c6x backend choose doloops whose tail and head are the same
> block, code generation will not be affected.
> But it may bring some negetive effect on target code efficiency for
> bfin backend as less hw-doloops may be generated.
> In my point of view, what we need here is a better block reordering
> that cares about hw-doloops.
> If OK for trunk, can someone please apply it? Thanks.
>
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog    (revision 206273)
> +++ gcc/ChangeLog    (working copy)
> @@ -1,3 +1,12 @@
> +2014-01-01  Felix Yang  <fei.yang0953@gmail.com>
> +
> +    * hw-doloop.c (set_bb_indices): Remove.
> +    (reorder_loops): Ditto.
> +    (reorg_loops): Remove do_reorder parameter.
> +    * hw-doloop.h (reorg_loops): Likewise.
> +    * config/c6x/c6x.c (c6x_hwloops): Do not rely on reorder_loops.
> +    * config/bfin/bfin.c (bfin_reorg_loops): Ditto.
> +
>  2014-01-01  Jakub Jelinek  <jakub@redhat.com>
>
>      * config/i386/sse.md (*mov<mode>_internal): Guard
> Index: gcc/hw-doloop.c
> ===================================================================
> --- gcc/hw-doloop.c    (revision 206273)
> +++ gcc/hw-doloop.c    (working copy)
> @@ -470,85 +470,6 @@ free_loops (hwloop_info loops)
>      }
>  }
>
> -#define BB_AUX_INDEX(BB) ((intptr_t) (BB)->aux)
> -
> -/* Initialize the aux fields to give ascending indices to basic blocks.  */
> -static void
> -set_bb_indices (void)
> -{
> -  basic_block bb;
> -  intptr_t index;
> -
> -  index = 0;
> -  FOR_EACH_BB_FN (bb, cfun)
> -    bb->aux = (void *) index++;
> -}
> -
> -/* The taken-branch edge from the loop end can actually go forward.
> -   If the target's hardware loop support requires that the loop end be
> -   after the loop start, try to reorder a loop's basic blocks when we
> -   find such a case.
> -   This is not very aggressive; it only moves at most one block.  It
> -   does not introduce new branches into loops; it may remove them, or
> -   it may switch fallthru/jump edges.  */
> -static void
> -reorder_loops (hwloop_info loops)
> -{
> -  basic_block bb;
> -  hwloop_info loop;
> -
> -  cfg_layout_initialize (0);
> -
> -  set_bb_indices ();
> -
> -  for (loop = loops; loop; loop = loop->next)
> -    {
> -      edge e;
> -      edge_iterator ei;
> -
> -      if (loop->bad)
> -    continue;
> -
> -      if (BB_AUX_INDEX (loop->head) <= BB_AUX_INDEX (loop->tail))
> -    continue;
> -
> -      FOR_EACH_EDGE (e, ei, loop->head->succs)
> -    {
> -      if (bitmap_bit_p (loop->block_bitmap, e->dest->index)
> -          && BB_AUX_INDEX (e->dest) < BB_AUX_INDEX (loop->tail))
> -        {
> -          basic_block start_bb = e->dest;
> -          basic_block start_prev_bb = start_bb->prev_bb;
> -
> -          if (dump_file)
> -        fprintf (dump_file, ";; Moving block %d before block %d\n",
> -             loop->head->index, start_bb->index);
> -          loop->head->prev_bb->next_bb = loop->head->next_bb;
> -          loop->head->next_bb->prev_bb = loop->head->prev_bb;
> -
> -          loop->head->prev_bb = start_prev_bb;
> -          loop->head->next_bb = start_bb;
> -          start_prev_bb->next_bb = start_bb->prev_bb = loop->head;
> -
> -          set_bb_indices ();
> -          break;
> -        }
> -    }
> -      loops = loops->next;
> -    }
> -
> -  FOR_EACH_BB_FN (bb, cfun)
> -    {
> -      if (bb->next_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
> -    bb->aux = bb->next_bb;
> -      else
> -    bb->aux = NULL;
> -    }
> -  cfg_layout_finalize ();
> -  clear_aux_for_blocks ();
> -  df_analyze ();
> -}
> -
>  /* Call the OPT function for LOOP and all of its sub-loops.  This is
>     done in a depth-first search; innermost loops are visited first.
>     OPTIMIZE and FAIL are the functions passed to reorg_loops by the
> @@ -611,15 +532,10 @@ optimize_loop (hwloop_info loop, struct hw_doloop_
>     instructions before the loop, to define loop bounds or set up a
>     special loop counter register.
>
> -   DO_REORDER should be set to true if we should try to use the
> -   reorder_loops function to ensure the loop end occurs after the loop
> -   start.  This is for use by targets where the loop hardware requires
> -   this condition.
> -
>     HOOKS is used to pass in target specific hooks; see
>     hw-doloop.h.  */
>  void
> -reorg_loops (bool do_reorder, struct hw_doloop_hooks *hooks)
> +reorg_loops (struct hw_doloop_hooks *hooks)
>  {
>    hwloop_info loops = NULL;
>    hwloop_info loop;
> @@ -632,21 +548,10 @@ void
>    bitmap_obstack_initialize (&loop_stack);
>
>    if (dump_file)
> -    fprintf (dump_file, ";; Find loops, first pass\n\n");
> +    fprintf (dump_file, ";; Find loops:\n\n");
>
>    loops = discover_loops (&loop_stack, hooks);
>
> -  if (do_reorder)
> -    {
> -      reorder_loops (loops);
> -      free_loops (loops);
> -
> -      if (dump_file)
> -    fprintf (dump_file, ";; Find loops, second pass\n\n");
> -
> -      loops = discover_loops (&loop_stack, hooks);
> -    }
> -
>    for (loop = loops; loop; loop = loop->next)
>      scan_loop (loop);
>
> Index: gcc/hw-doloop.h
> ===================================================================
> --- gcc/hw-doloop.h    (revision 206273)
> +++ gcc/hw-doloop.h    (working copy)
> @@ -152,4 +152,4 @@ struct hw_doloop_hooks
>    void (*fail) (hwloop_info loop);
>  };
>
> -extern void reorg_loops (bool, struct hw_doloop_hooks *);
> +extern void reorg_loops (struct hw_doloop_hooks *);
> Index: gcc/config/c6x/c6x.c
> ===================================================================
> --- gcc/config/c6x/c6x.c    (revision 206273)
> +++ gcc/config/c6x/c6x.c    (working copy)
> @@ -5895,7 +5895,7 @@ static void
>  c6x_hwloops (void)
>  {
>    if (optimize)
> -    reorg_loops (true, &c6x_doloop_hooks);
> +    reorg_loops (&c6x_doloop_hooks);
>  }
>
>  /* Implement the TARGET_MACHINE_DEPENDENT_REORG pass.  We split call insns here
> Index: gcc/config/bfin/bfin.c
> ===================================================================
> --- gcc/config/bfin/bfin.c    (revision 206273)
> +++ gcc/config/bfin/bfin.c    (working copy)
> @@ -3885,7 +3885,7 @@ static struct hw_doloop_hooks bfin_doloop_hooks =
>  static void
>  bfin_reorg_loops (void)
>  {
> -  reorg_loops (true, &bfin_doloop_hooks);
> +  reorg_loops (&bfin_doloop_hooks);
>  }
>
>  /* Possibly generate a SEQUENCE out of three insns found in SLOT.
>
>
> Cheers,
> Felix