diff mbox series

middle-end: Fix phi-ssa assertion triggers. [PR106519]

Message ID patch-16119-tamar@arm.com
State New, archived
Headers
Series middle-end: Fix phi-ssa assertion triggers. [PR106519] |

Commit Message

Tamar Christina Aug. 4, 2022, 12:17 p.m. UTC 
  Hi All,

The failures on -m32 on x86 show that the checks at the top level in
tree_ssa_phiopt_worker aren't enough for diamond_p.

In minmax_replacement we perform the additional validation of the shape but by
then it's too late to catch these case.

This patch changes it so we check that for a diamond shape we check that the
edges we're operation on must result in the same destination BB.

We also enforce that for a diamond the middle BBs must have a single successor,
this is because the remainder of the code always follows EDGE_SUCC 0.  If there
are more than 1 successor then the edge we follow could be not part of the
diamond so just reject it early on.

I also remove the assert after the use of gimple_phi_arg_def as this function
can't return NULL. if the index is out of range it already breaks on an assert
inside the gimple_phi_arg_def, so we never hit this assert.

Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
and no issues.

Ok for master?

Thanks,
Tamar

gcc/ChangeLog:

	PR middle-end/106519
	* tree-ssa-phiopt.cc (tree_ssa_phiopt_worker): Check final phi edge for
	diamond shapes.

gcc/testsuite/ChangeLog:

	PR middle-end/106519
	* gcc.dg/pr106519.c: New test.

--- inline copy of patch -- 
diff --git a/gcc/testsuite/gcc.dg/pr106519.c b/gcc/testsuite/gcc.dg/pr106519.c
new file mode 100644
index 0000000000000000000000000000000000000000..3d4662d8a02c6501560abb71ac53320f093620d8




--
diff --git a/gcc/testsuite/gcc.dg/pr106519.c b/gcc/testsuite/gcc.dg/pr106519.c
new file mode 100644
index 0000000000000000000000000000000000000000..3d4662d8a02c6501560abb71ac53320f093620d8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr106519.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+
+int bytestart, bytemem_0_0, modlookup_l_p;
+
+void
+modlookup_l() {
+  long j;
+  while (modlookup_l_p)
+    while (bytestart && j && bytemem_0_0)
+      j = j + 1;
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index a8e55e040649e17f83a2fc3340e368cf9c4c5e70..1c4942b5b5c18732a8b18789c04e2685437404dd 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -268,7 +268,8 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  continue;
 	}
       else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest
-	       && !empty_block_p (bb1))
+	       && !empty_block_p (bb1)
+	       && single_succ_p (bb1) && single_succ_p (bb2))
 	diamond_p = true;
       else
 	continue;
@@ -311,6 +312,12 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	    for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
 	      {
 		phi = as_a <gphi *> (gsi_stmt (gsi));
+
+		/* A diamond must continue to the same destination node, otherwise
+		   by definition it's not a diamond.  */
+		if (diamond_p && e1->dest != e2->dest)
+		  continue;
+
 		arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
 		arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
 		if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) == 2)
@@ -329,12 +336,15 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  if (!phi)
 	    continue;
 
-	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
-	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
+	  /* A diamond must continue to the same destination node, otherwise
+	     by definition it's not a diamond.  */
+	  if (diamond_p && e1->dest != e2->dest)
+	    continue;
 
 	  /* Something is wrong if we cannot find the arguments in the PHI
 	     node.  */
-	  gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
+	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
+	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
 
 	  gphi *newphi;
 	  if (single_pred_p (bb1)

Comments

Richard Biener Aug. 4, 2022, 12:49 p.m. UTC | #1 
On Thu, 4 Aug 2022, Tamar Christina wrote:

> Hi All,
> 
> The failures on -m32 on x86 show that the checks at the top level in
> tree_ssa_phiopt_worker aren't enough for diamond_p.
> 
> In minmax_replacement we perform the additional validation of the shape but by
> then it's too late to catch these case.
> 
> This patch changes it so we check that for a diamond shape we check that the
> edges we're operation on must result in the same destination BB.
> 
> We also enforce that for a diamond the middle BBs must have a single successor,
> this is because the remainder of the code always follows EDGE_SUCC 0.  If there
> are more than 1 successor then the edge we follow could be not part of the
> diamond so just reject it early on.
> 
> I also remove the assert after the use of gimple_phi_arg_def as this function
> can't return NULL. if the index is out of range it already breaks on an assert
> inside the gimple_phi_arg_def, so we never hit this assert.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> and no issues.
> 
> Ok for master?
> 
> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/106519
> 	* tree-ssa-phiopt.cc (tree_ssa_phiopt_worker): Check final phi edge for
> 	diamond shapes.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/106519
> 	* gcc.dg/pr106519.c: New test.
> 
> --- inline copy of patch -- 
> diff --git a/gcc/testsuite/gcc.dg/pr106519.c b/gcc/testsuite/gcc.dg/pr106519.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..3d4662d8a02c6501560abb71ac53320f093620d8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr106519.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +
> +int bytestart, bytemem_0_0, modlookup_l_p;
> +
> +void
> +modlookup_l() {
> +  long j;
> +  while (modlookup_l_p)
> +    while (bytestart && j && bytemem_0_0)
> +      j = j + 1;
> +}
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index a8e55e040649e17f83a2fc3340e368cf9c4c5e70..1c4942b5b5c18732a8b18789c04e2685437404dd 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -268,7 +268,8 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>  	  continue;
>  	}
>        else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest
> -	       && !empty_block_p (bb1))
> +	       && !empty_block_p (bb1)
> +	       && single_succ_p (bb1) && single_succ_p (bb2))
>  	diamond_p = true;
>        else
>  	continue;
> @@ -311,6 +312,12 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>  	    for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
>  	      {
>  		phi = as_a <gphi *> (gsi_stmt (gsi));
> +
> +		/* A diamond must continue to the same destination node, otherwise
> +		   by definition it's not a diamond.  */
> +		if (diamond_p && e1->dest != e2->dest)

this and the check below looks redundant since we already check
EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest when setting
diamond_p = true?  Which means we already verify that following
edge 0 we have a diamond?

In fact for the testcase we _do_ have a diamond but we failed to
adjust e2 to point to the merge block early, we only do it here:

          e2 = diamond_p ? EDGE_SUCC (bb2, 0) : e2;
          phi = single_non_singleton_phi_for_edges (phis, e1, e2);

That also means that the do_store_elim case possibly wants a
bail out on diamond_p?  Not sure for value_replacement.

At least

diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index a8e55e04064..ef4c0b78f4e 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -269,7 +269,10 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool 
do_hoist_loads, bool early_p)
        }
       else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest
               && !empty_block_p (bb1))
-       diamond_p = true;
+       {
+         diamond_p = true;
+         e2 = EDGE_SUCC (bb2, 0);
+       }
       else
        continue;
 
@@ -324,7 +327,6 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool 
do_hoist_loads, bool early_p)
          if (!candorest)
            continue;
 
-         e2 = diamond_p ? EDGE_SUCC (bb2, 0) : e2;
          phi = single_non_singleton_phi_for_edges (phis, e1, e2);
          if (!phi)
            continue;

fixes the ICE for me but I did no further checking.

> +		  continue;
> +
>  		arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
>  		arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
>  		if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) == 2)
> @@ -329,12 +336,15 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>  	  if (!phi)
>  	    continue;
>  
> -	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> -	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> +	  /* A diamond must continue to the same destination node, otherwise
> +	     by definition it's not a diamond.  */
> +	  if (diamond_p && e1->dest != e2->dest)
> +	    continue;
>  
>  	  /* Something is wrong if we cannot find the arguments in the PHI
>  	     node.  */
> -	  gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> +	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> +	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
>  
>  	  gphi *newphi;
>  	  if (single_pred_p (bb1)
> 
> 
> 
> 
>
diff mbox series

Patch

--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr106519.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+
+int bytestart, bytemem_0_0, modlookup_l_p;
+
+void
+modlookup_l() {
+  long j;
+  while (modlookup_l_p)
+    while (bytestart && j && bytemem_0_0)
+      j = j + 1;
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index a8e55e040649e17f83a2fc3340e368cf9c4c5e70..1c4942b5b5c18732a8b18789c04e2685437404dd 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -268,7 +268,8 @@  tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  continue;
 	}
       else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest
-	       && !empty_block_p (bb1))
+	       && !empty_block_p (bb1)
+	       && single_succ_p (bb1) && single_succ_p (bb2))
 	diamond_p = true;
       else
 	continue;
@@ -311,6 +312,12 @@  tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	    for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
 	      {
 		phi = as_a <gphi *> (gsi_stmt (gsi));
+
+		/* A diamond must continue to the same destination node, otherwise
+		   by definition it's not a diamond.  */
+		if (diamond_p && e1->dest != e2->dest)
+		  continue;
+
 		arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
 		arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
 		if (value_replacement (bb, bb1, e1, e2, phi, arg0, arg1) == 2)
@@ -329,12 +336,15 @@  tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
 	  if (!phi)
 	    continue;
 
-	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
-	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
+	  /* A diamond must continue to the same destination node, otherwise
+	     by definition it's not a diamond.  */
+	  if (diamond_p && e1->dest != e2->dest)
+	    continue;
 
 	  /* Something is wrong if we cannot find the arguments in the PHI
 	     node.  */
-	  gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
+	  arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
+	  arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
 
 	  gphi *newphi;
 	  if (single_pred_p (bb1)