* [PATCH]middle-end: Fix phi-ssa assertion triggers. [PR106519]
@ 2022-08-04 12:17 Tamar Christina
2022-08-04 12:49 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Tamar Christina @ 2022-08-04 12:17 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, rguenther
[-- Attachment #1: Type: text/plain, Size: 3537 bytes --]
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)
+ 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)
--
[-- Attachment #2: rb16119.patch --]
[-- Type: text/plain, Size: 2292 bytes --]
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)
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH]middle-end: Fix phi-ssa assertion triggers. [PR106519]
2022-08-04 12:17 [PATCH]middle-end: Fix phi-ssa assertion triggers. [PR106519] Tamar Christina
@ 2022-08-04 12:49 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2022-08-04 12:49 UTC (permalink / raw)
To: Tamar Christina; +Cc: gcc-patches, nd
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)
>
>
>
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-08-04 12:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-04 12:17 [PATCH]middle-end: Fix phi-ssa assertion triggers. [PR106519] Tamar Christina
2022-08-04 12:49 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).