Re: [PATCH] Fix bb-reorder ICEs (PR rtl-optimization/80747, PR rtl-optimization/83512)

[Richard Biener <richard dot guenther at gmail dot com>]

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