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

[Felix Yang]

Ping?
Cheers,
Felix


On Thu, Jan 2, 2014 at 10:13 PM, Felix Yang <fei.yang0953@gmail.com> wrote:
> Hi Bernd,
>
>   It's easy to reproduce this error:
>   step1: Build gcc for c6x from gcc code of latest trunk, say
> c6x-uclinux-gcc for example
>   step2: Using the newly built c6x gcc compiler to comiple this small case:
>             $c6x-unlinux-gcc -S -O2 arraysum.c
>
> arraysum.c:
>
> int array[1024];
>
> int test_entry ()
> {
>     unsigned int i;
>     int sum = 0;
>
>     for (i = 0; i < 1024; i++) {
>         sum = sum + array[i];
>     }
>
>     return sum;
> }
>
> Note: -freorder-blocks is enable under -O2.
>
> I think it would be better to skip reorder_loops if bb reordering is
> enabled, since function without hw-doloops
> can still benefit from block reordering optimization.
> Attached please find the updated patch. Please apply it if OK for trunk. Thanks
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog    (revision 206280)
> +++ gcc/ChangeLog    (working copy)
> @@ -1,3 +1,9 @@
> +2014-01-02  Felix Yang  <fei.yang0953@gmail.com>
> +
> +    * config/c6x/c6x.c (c6x_hwloops): Do not reorder loop's basic blocks
> +    if bb reordering is enabled.
> +    * config/bfin/bfin.c (bfin_reorg_loops): Ditto.
> +
>  2014-01-01  Jan-Benedict Glaw  <jbglaw@lug-owl.de>
>
>      * config/nios2/nios2.h (BITS_PER_UNIT): Don't define it.
> Index: gcc/config/c6x/c6x.c
> ===================================================================
> --- gcc/config/c6x/c6x.c    (revision 206280)
> +++ gcc/config/c6x/c6x.c    (working copy)
> @@ -5895,7 +5895,15 @@ static void
>  c6x_hwloops (void)
>  {
>    if (optimize)
> -    reorg_loops (true, &c6x_doloop_hooks);
> +    {
> +      /* Do not reorder loop's basic blocks if bb reordering is
> enabled. The reason
> +         is that cfg layout mode should not be re-entered once bb reordering is
> +         complete.  */
> +      if (flag_reorder_blocks || flag_reorder_blocks_and_partition)
> +        reorg_loops (false, &c6x_doloop_hooks);
> +      else
> +        reorg_loops (true, &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 206280)
> +++ gcc/config/bfin/bfin.c    (working copy)
> @@ -3885,7 +3885,13 @@ static struct hw_doloop_hooks bfin_doloop_hooks =
>  static void
>  bfin_reorg_loops (void)
>  {
> -  reorg_loops (true, &bfin_doloop_hooks);
> +  /* Do not reorder loop's basic blocks if bb reordering is enabled. The reason
> +     is that cfg layout mode should not be re-entered once bb reordering is
> +     complete.  */
> +  if (flag_reorder_blocks || flag_reorder_blocks_and_partition)
> +    reorg_loops (false, &bfin_doloop_hooks);
> +  else
> +    reorg_loops (true, &bfin_doloop_hooks);
>  }
>
>  /* Possibly generate a SEQUENCE out of three insns found in SLOT.
>
> Cheers,
> Felix
>
>
> On Thu, Jan 2, 2014 at 7:04 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>> On 01/01/2014 02:01 PM, Felix Yang wrote:
>>>
>>> cc1 backtrace:
>>>
>>> arraysum.c: In function 'test_entry':
>>> arraysum.c:14:1: internal compiler error: in cfg_layout_initialize, at
>>> cfgrtl.c:4233
>>
>>
>> Please include steps to reproduce bugs.
>>
>>
>>> 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.
>>
>>
>> Is this with -freorder-blocks? Rather than delete the code, just skip it if
>> that flag is enabled. Or forcibly turn it off for these ports in
>> option_override.
>>
>>
>> Bernd
>>