| Message ID | patch-16119-tamar@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end: Fix phi-ssa assertion triggers. [PR106519] | expand |
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) > > > > >
--- /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)
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)