[tail-merge,PR84956] Don't merge bbs with bb_has_abnormal_pred

Message ID 3bfab45a-2a04-e834-8af6-3390c8147347@mentor.com
State New
Headers show
Series
  • [tail-merge,PR84956] Don't merge bbs with bb_has_abnormal_pred
Related show

Commit Message

Tom de Vries March 22, 2018, 9:54 a.m.
Hi,

This fixes a 6/7/8 regression, a P3 ICE in the tail-merge pass.


Consider the test-case from the patch.

When compiling at -O2, duplicates are found in tail-merge:
...
find_duplicates: <bb 6> duplicate of <bb 7>
find_duplicates: <bb 6> duplicate of <bb 9>
...
So, tail-merge calls replace_block_by (bb7, bb6).

However, bb7 has an abnormal edge bb5 -> bb7 as predecessor (bb6 has an 
abnormal edge as predecessor as well, but that doesn't look necessary to 
trigger the ICE):
...
;;   basic block 9, loop depth 0
;;    prev block 3, next block 4, flags: (NEW, REACHABLE)
;;    pred:       3 [50.0% (guessed)] (FALSE_VALUE,EXECUTABLE)
   goto <bb 8>; [100.00%]
;;    succ:       8 [always]  (FALLTHRU,EXECUTABLE)

;;   basic block 6, loop depth 0
;;    prev block 5, next block 7, flags: (NEW, REACHABLE, VISITED)
;;    pred:       5 [33.3% (guessed)] (ABNORMAL,EXECUTABLE)
   goto <bb 8>; [100.00%]
;;    succ:       8 [always] (FALLTHRU,EXECUTABLE)

;;   basic block 7, loop depth 0
;;    prev block 6, next block 8, flags: (NEW, REACHABLE, VISITED)
;;    pred:       5 [33.3% (guessed)]  (ABNORMAL,EXECUTABLE)
;;    succ:       8 [always]  (FALLTHRU,EXECUTABLE)
...

So when replace_block_by calls redirect_edge_and_branch (bb5 -> bb7, 
bb6) it lands in gimple_redirect_edge_and_branch, and fails here:
...
6024	  if (e->flags & EDGE_ABNORMAL)
6025	    return NULL;
...

which causes this assert to trigger:
...
       gcc_assert (pred_edge != NULL);
...


The patch fixes the ICE conservatively by skipping bbs with 
bb_has_abnormal_preds in find_clusters_1.

[ A more optimal fix could be to detect in this example that there's an 
abnormal edge bb5 -> bb6, and skip redirecting bb5 -> bb7 to bb6. ]

Bootstrapped and reg-tested on x86_64.

OK for stage4, gcc-6-branch and gcc-7-branch?

Thanks,
- Tom

Comments

Richard Biener March 22, 2018, 10:11 a.m. | #1
On Thu, 22 Mar 2018, Tom de Vries wrote:

> Hi,

> 

> This fixes a 6/7/8 regression, a P3 ICE in the tail-merge pass.

> 

> 

> Consider the test-case from the patch.

> 

> When compiling at -O2, duplicates are found in tail-merge:

> ...

> find_duplicates: <bb 6> duplicate of <bb 7>

> find_duplicates: <bb 6> duplicate of <bb 9>

> ...

> So, tail-merge calls replace_block_by (bb7, bb6).

> 

> However, bb7 has an abnormal edge bb5 -> bb7 as predecessor (bb6 has an

> abnormal edge as predecessor as well, but that doesn't look necessary to

> trigger the ICE):

> ...

> ;;   basic block 9, loop depth 0

> ;;    prev block 3, next block 4, flags: (NEW, REACHABLE)

> ;;    pred:       3 [50.0% (guessed)] (FALSE_VALUE,EXECUTABLE)

>   goto <bb 8>; [100.00%]

> ;;    succ:       8 [always]  (FALLTHRU,EXECUTABLE)

> 

> ;;   basic block 6, loop depth 0

> ;;    prev block 5, next block 7, flags: (NEW, REACHABLE, VISITED)

> ;;    pred:       5 [33.3% (guessed)] (ABNORMAL,EXECUTABLE)

>   goto <bb 8>; [100.00%]

> ;;    succ:       8 [always] (FALLTHRU,EXECUTABLE)

> 

> ;;   basic block 7, loop depth 0

> ;;    prev block 6, next block 8, flags: (NEW, REACHABLE, VISITED)

> ;;    pred:       5 [33.3% (guessed)]  (ABNORMAL,EXECUTABLE)

> ;;    succ:       8 [always]  (FALLTHRU,EXECUTABLE)

> ...

> 

> So when replace_block_by calls redirect_edge_and_branch (bb5 -> bb7, bb6) it

> lands in gimple_redirect_edge_and_branch, and fails here:

> ...

> 6024	  if (e->flags & EDGE_ABNORMAL)

> 6025	    return NULL;

> ...

> 

> which causes this assert to trigger:

> ...

>       gcc_assert (pred_edge != NULL);

> ...

> 

> 

> The patch fixes the ICE conservatively by skipping bbs with

> bb_has_abnormal_preds in find_clusters_1.

> 

> [ A more optimal fix could be to detect in this example that there's an

> abnormal edge bb5 -> bb6, and skip redirecting bb5 -> bb7 to bb6. ]

> 

> Bootstrapped and reg-tested on x86_64.

> 

> OK for stage4, gcc-6-branch and gcc-7-branch?


OK everywhere.

Thanks,
Richard.

Patch

[tail-merge] Don't merge bbs with bb_has_abnormal_pred

2018-03-21  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/84956
	* tree-ssa-tail-merge.c (find_clusters_1): Skip bbs with
	bb_has_abnormal_pred.

	* gcc.dg/pr84956.c: New test.

---
 gcc/testsuite/gcc.dg/pr84956.c | 27 +++++++++++++++++++++++++++
 gcc/tree-ssa-tail-merge.c      |  6 ++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/pr84956.c b/gcc/testsuite/gcc.dg/pr84956.c
new file mode 100644
index 0000000..055a749
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr84956.c
@@ -0,0 +1,27 @@ 
+/* { dg-options "-O2 -ftree-tail-merge" } */
+
+char a;
+int c;
+unsigned b ();
+
+unsigned
+setjmp ()
+{
+}
+
+static void
+d ()
+{
+  if (b ())
+    c = 3;
+}
+
+void
+e ()
+{
+  d ();
+  a && ({ setjmp (); });
+  a && ({ setjmp (); });
+  a && ({ setjmp (); });
+}
+
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index a687c3f..f482ce1 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -1455,7 +1455,8 @@  find_clusters_1 (same_succ *same_succ)
       /* TODO: handle blocks with phi-nodes.  We'll have to find corresponding
 	 phi-nodes in bb1 and bb2, with the same alternatives for the same
 	 preds.  */
-      if (bb_has_non_vop_phi (bb1) || bb_has_eh_pred (bb1))
+      if (bb_has_non_vop_phi (bb1) || bb_has_eh_pred (bb1)
+	  || bb_has_abnormal_pred (bb1))
 	continue;
 
       nr_comparisons = 0;
@@ -1463,7 +1464,8 @@  find_clusters_1 (same_succ *same_succ)
 	{
 	  bb2 = BASIC_BLOCK_FOR_FN (cfun, j);
 
-	  if (bb_has_non_vop_phi (bb2) || bb_has_eh_pred (bb2))
+	  if (bb_has_non_vop_phi (bb2) || bb_has_eh_pred (bb2)
+	      || bb_has_abnormal_pred (bb2))
 	    continue;
 
 	  if (BB_CLUSTER (bb1) != NULL && BB_CLUSTER (bb1) == BB_CLUSTER (bb2))