Fix bb-reorder ICEs (PR rtl-optimization/80747, PR rtl-optimization/83512)

Message ID 20171221180510.GC2353@tucnak
State New
Headers show
Series
  • Fix bb-reorder ICEs (PR rtl-optimization/80747, PR rtl-optimization/83512)
Related show

Commit Message

Jakub Jelinek Dec. 21, 2017, 6:05 p.m.
Hi!

The following two testcases show multiple issues in
reorder_basic_blocks_simple.  Both issues only occur if the greedy algorithm
decides to put some basic block before the ENTRY successor.  In that case
reorder_basic_blocks_simple has code to split the ENTRY successor edge
and put the new bb before all the others.  If pass_partition_blocks
was done earlier, one issue is that in this case (when the ENTRY successor
edge was ignored by the greedy algorithm) ENTRY_BLOCK_PTR_FOR_FN (cfun)->aux
is the ENTRY block itself, and only the non-fixed basic blocks are cold or
hot, not ENTRY/EXIT, so starting with current_partition set to BB_PARTITION
of the ENTRY block doesn't work - it will be BB_UNPARITIONED and so neither
of the passes of the following loop will do anything.  IMHO we want to
use the partition of the ENTRY successor bb, whether it is hot or cold
(typically hot).  The other issue is that when we force_nonfallthru
on the ENTRY successor edge, e->src is BB_UNPARTITIONED, e->dest if
bbpart was done is either hot or cold (but for these edges we of course
never set EDGE_CROSSING).  force_nonfallthru_and_redirect creates a new
bb which will be BB_UNPARTITIONED and so the new edge is EDGE_CROSSING
and the new jump is CROSSING_JUMP_P.  Then reorder_basic_blocks_simple
fixes up the partition of the new bb through BB_COPY_PARTITION, but it is
too late, it would need to undo EDGE_CROSSING flag on the edge and
CROSSING_JUMP_P too.  IMHO it is better to get the partition right from the
beginning, then we don't have to undo anything.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-12-21  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/80747
	PR rtl-optimization/83512
	* cfgrtl.c (force_nonfallthru_and_redirect): When splitting
	succ edge from ENTRY, copy partition from e->dest to the newly
	created bb.
	* bb-reorder.c (reorder_basic_blocks_simple): If last_tail is
	ENTRY, use BB_PARTITION of its successor block as current_partition.
	Don't copy partition when splitting succ edge from ENTRY.

	* gcc.dg/pr80747.c: New test.
	* gcc.dg/pr83512.c: New test.


	Jakub

Comments

Richard Biener Dec. 21, 2017, 6:55 p.m. | #1
On December 21, 2017 7:05:10 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!

>

>The following two testcases show multiple issues in

>reorder_basic_blocks_simple.  Both issues only occur if the greedy

>algorithm

>decides to put some basic block before the ENTRY successor.  In that

>case

>reorder_basic_blocks_simple has code to split the ENTRY successor edge

>and put the new bb before all the others.  If pass_partition_blocks

>was done earlier, one issue is that in this case (when the ENTRY

>successor

>edge was ignored by the greedy algorithm) ENTRY_BLOCK_PTR_FOR_FN

>(cfun)->aux

>is the ENTRY block itself, and only the non-fixed basic blocks are cold

>or

>hot, not ENTRY/EXIT, so starting with current_partition set to

>BB_PARTITION

>of the ENTRY block doesn't work - it will be BB_UNPARITIONED and so

>neither

>of the passes of the following loop will do anything.  IMHO we want to

>use the partition of the ENTRY successor bb, whether it is hot or cold

>(typically hot).  The other issue is that when we force_nonfallthru

>on the ENTRY successor edge, e->src is BB_UNPARTITIONED, e->dest if

>bbpart was done is either hot or cold (but for these edges we of course

>never set EDGE_CROSSING).  force_nonfallthru_and_redirect creates a new

>bb which will be BB_UNPARTITIONED and so the new edge is EDGE_CROSSING

>and the new jump is CROSSING_JUMP_P.  Then reorder_basic_blocks_simple

>fixes up the partition of the new bb through BB_COPY_PARTITION, but it

>is

>too late, it would need to undo EDGE_CROSSING flag on the edge and

>CROSSING_JUMP_P too.  IMHO it is better to get the partition right from

>the

>beginning, then we don't have to undo anything.

>

>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


OK. 

Richard. 

>2017-12-21  Jakub Jelinek  <jakub@redhat.com>

>

>	PR rtl-optimization/80747

>	PR rtl-optimization/83512

>	* cfgrtl.c (force_nonfallthru_and_redirect): When splitting

>	succ edge from ENTRY, copy partition from e->dest to the newly

>	created bb.

>	* bb-reorder.c (reorder_basic_blocks_simple): If last_tail is

>	ENTRY, use BB_PARTITION of its successor block as current_partition.

>	Don't copy partition when splitting succ edge from ENTRY.

>

>	* gcc.dg/pr80747.c: New test.

>	* gcc.dg/pr83512.c: New test.

>

>--- gcc/cfgrtl.c.jj	2017-12-20 20:40:19.000000000 +0100

>+++ gcc/cfgrtl.c	2017-12-21 16:25:24.172698221 +0100

>@@ -1534,6 +1534,9 @@ force_nonfallthru_and_redirect (edge e,

> 					       ENTRY_BLOCK_PTR_FOR_FN (cfun));

> 	  bb->count = ENTRY_BLOCK_PTR_FOR_FN (cfun)->count;

> 

>+	  /* Make sure new block ends up in correct hot/cold section.  */

>+	  BB_COPY_PARTITION (bb, e->dest);

>+

> 	  /* Change the existing edge's source to be the new block, and add

> 	     a new edge from the entry block to the new block.  */

> 	  e->src = bb;

>--- gcc/bb-reorder.c.jj	2017-11-22 23:34:45.000000000 +0100

>+++ gcc/bb-reorder.c	2017-12-21 16:25:54.237319354 +0100

>@@ -2405,7 +2405,10 @@ reorder_basic_blocks_simple (void)

> 

>basic_block last_tail = (basic_block) ENTRY_BLOCK_PTR_FOR_FN

>(cfun)->aux;

> 

>-  int current_partition = BB_PARTITION (last_tail);

>+  int current_partition

>+    = BB_PARTITION (last_tail == ENTRY_BLOCK_PTR_FOR_FN (cfun)

>+		    ? EDGE_SUCC (ENTRY_BLOCK_PTR_FOR_FN (cfun), 0)->dest

>+		    : last_tail);

>   bool need_another_pass = true;

> 

>   for (int pass = 0; pass < 2 && need_another_pass; pass++)

>@@ -2446,7 +2449,6 @@ reorder_basic_blocks_simple (void)

>     {

>       force_nonfallthru (e);

>       e->src->aux = ENTRY_BLOCK_PTR_FOR_FN (cfun)->aux;

>-      BB_COPY_PARTITION (e->src, e->dest);

>     }

> }

> 

>--- gcc/testsuite/gcc.dg/pr80747.c.jj	2017-12-21 16:27:18.234260846

>+0100

>+++ gcc/testsuite/gcc.dg/pr80747.c	2017-12-21 16:26:56.000000000 +0100

>@@ -0,0 +1,18 @@

>+/* PR rtl-optimization/80747 */

>+/* { dg-do compile } */

>+/* { dg-options "-fprofile-use -freorder-blocks-and-partition -O1

>-foptimize-sibling-calls" } */

>+

>+int

>+foo (int a)

>+{

>+  int r;

>+  if (a & 1)

>+    r = foo (a - 1);

>+  else if (a)

>+    r = foo (a - 2);

>+  else

>+    return 0;

>+  if (r)

>+    r = r;

>+  return r;

>+}

>--- gcc/testsuite/gcc.dg/pr83512.c.jj	2017-12-21 16:41:11.487769515

>+0100

>+++ gcc/testsuite/gcc.dg/pr83512.c	2017-12-21 16:40:50.000000000 +0100

>@@ -0,0 +1,16 @@

>+/* PR rtl-optimization/83512 */

>+/* { dg-do compile } */

>+/* { dg-options "-O2 -freorder-blocks-algorithm=simple" } */

>+

>+int a;

>+

>+void

>+foo (int *x)

>+{

>+  for (;;)

>+    {

>+      for (*x = 0; *x < 1; *x++)

>+	;

>+      ++a;

>+    }

>+}

>

>	Jakub

Patch

--- gcc/cfgrtl.c.jj	2017-12-20 20:40:19.000000000 +0100
+++ gcc/cfgrtl.c	2017-12-21 16:25:24.172698221 +0100
@@ -1534,6 +1534,9 @@  force_nonfallthru_and_redirect (edge e,
 					       ENTRY_BLOCK_PTR_FOR_FN (cfun));
 	  bb->count = ENTRY_BLOCK_PTR_FOR_FN (cfun)->count;
 
+	  /* Make sure new block ends up in correct hot/cold section.  */
+	  BB_COPY_PARTITION (bb, e->dest);
+
 	  /* Change the existing edge's source to be the new block, and add
 	     a new edge from the entry block to the new block.  */
 	  e->src = bb;
--- gcc/bb-reorder.c.jj	2017-11-22 23:34:45.000000000 +0100
+++ gcc/bb-reorder.c	2017-12-21 16:25:54.237319354 +0100
@@ -2405,7 +2405,10 @@  reorder_basic_blocks_simple (void)
 
   basic_block last_tail = (basic_block) ENTRY_BLOCK_PTR_FOR_FN (cfun)->aux;
 
-  int current_partition = BB_PARTITION (last_tail);
+  int current_partition
+    = BB_PARTITION (last_tail == ENTRY_BLOCK_PTR_FOR_FN (cfun)
+		    ? EDGE_SUCC (ENTRY_BLOCK_PTR_FOR_FN (cfun), 0)->dest
+		    : last_tail);
   bool need_another_pass = true;
 
   for (int pass = 0; pass < 2 && need_another_pass; pass++)
@@ -2446,7 +2449,6 @@  reorder_basic_blocks_simple (void)
     {
       force_nonfallthru (e);
       e->src->aux = ENTRY_BLOCK_PTR_FOR_FN (cfun)->aux;
-      BB_COPY_PARTITION (e->src, e->dest);
     }
 }
 
--- gcc/testsuite/gcc.dg/pr80747.c.jj	2017-12-21 16:27:18.234260846 +0100
+++ gcc/testsuite/gcc.dg/pr80747.c	2017-12-21 16:26:56.000000000 +0100
@@ -0,0 +1,18 @@ 
+/* PR rtl-optimization/80747 */
+/* { dg-do compile } */
+/* { dg-options "-fprofile-use -freorder-blocks-and-partition -O1 -foptimize-sibling-calls" } */
+
+int
+foo (int a)
+{
+  int r;
+  if (a & 1)
+    r = foo (a - 1);
+  else if (a)
+    r = foo (a - 2);
+  else
+    return 0;
+  if (r)
+    r = r;
+  return r;
+}
--- gcc/testsuite/gcc.dg/pr83512.c.jj	2017-12-21 16:41:11.487769515 +0100
+++ gcc/testsuite/gcc.dg/pr83512.c	2017-12-21 16:40:50.000000000 +0100
@@ -0,0 +1,16 @@ 
+/* PR rtl-optimization/83512 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -freorder-blocks-algorithm=simple" } */
+
+int a;
+
+void
+foo (int *x)
+{
+  for (;;)
+    {
+      for (*x = 0; *x < 1; *x++)
+	;
+      ++a;
+    }
+}