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

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

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
>

Attachment: bfin-c6x.diff
Description: Text document