PHIOPT: Fix diamond case of match_simplify_replacement
Checks
Commit Message
So it turns out I messed checking which edge was true/false for the diamond
form. The edges, e0 and e1 here are edges from the merge block but the
true/false edges are from the conditional block and with diamond/threeway,
there is a bb inbetween on both edges.
Most of the time, the check that was in match_simplify_replacement would
happen to be correct for diamond form as most of the time the first edge in
the conditional is the edge for the true side of the conditional.
This is why I didn't see the issue during bootstrap/testing.
I added a fragile gimple testcase which exposed the issue. Since there is
no way to specify the order of the edges in the gimple fe, we have to
have forwprop to swap the false/true edges (not order of them, just swapping
true/false flags) and hope not to do cleanupcfg inbetween forwprop and the
first phiopt pass. This is the fragile part really, it is not that we will
produce wrong code, just we won't hit what was the failing case.
OK? Bootstrapped and tested on x86_64-linux-gnu.
PR tree-optimization/109732
gcc/ChangeLog:
* tree-ssa-phiopt.cc (match_simplify_replacement): Fix the selection
of the argtrue/argfalse.
gcc/testsuite/ChangeLog:
* gcc.dg/pr109732.c: New test.
---
gcc/testsuite/gcc.dg/pr109732.c | 40 +++++++++++++++++++++++++++++++++
gcc/tree-ssa-phiopt.cc | 29 +++++++++++++++++++++---
2 files changed, 66 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/pr109732.c
Comments
On Thu, May 4, 2023 at 11:57 PM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> So it turns out I messed checking which edge was true/false for the diamond
> form. The edges, e0 and e1 here are edges from the merge block but the
> true/false edges are from the conditional block and with diamond/threeway,
> there is a bb inbetween on both edges.
> Most of the time, the check that was in match_simplify_replacement would
> happen to be correct for diamond form as most of the time the first edge in
> the conditional is the edge for the true side of the conditional.
> This is why I didn't see the issue during bootstrap/testing.
>
> I added a fragile gimple testcase which exposed the issue. Since there is
> no way to specify the order of the edges in the gimple fe, we have to
> have forwprop to swap the false/true edges (not order of them, just swapping
> true/false flags) and hope not to do cleanupcfg inbetween forwprop and the
> first phiopt pass. This is the fragile part really, it is not that we will
> produce wrong code, just we won't hit what was the failing case.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu.
OK.
Thanks,
Richard.
> PR tree-optimization/109732
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.cc (match_simplify_replacement): Fix the selection
> of the argtrue/argfalse.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/pr109732.c: New test.
> ---
> gcc/testsuite/gcc.dg/pr109732.c | 40 +++++++++++++++++++++++++++++++++
> gcc/tree-ssa-phiopt.cc | 29 +++++++++++++++++++++---
> 2 files changed, 66 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr109732.c
>
> diff --git a/gcc/testsuite/gcc.dg/pr109732.c b/gcc/testsuite/gcc.dg/pr109732.c
> new file mode 100644
> index 00000000000..d8374705cd8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr109732.c
> @@ -0,0 +1,40 @@
> +/* { dg-do run } */
> +/* We need to disable passes which might cause cfg cleanup */
> +/* { dg-options "-O1 -fgimple -fdisable-tree-ethread -fdisable-tree-fre1" } */
> +
> +/* This code is done this way to have the false edge as 1st
> + successor edge of BB2. Normally the true edge would be
> + the first and you would not hit the bug. */
> +[[gnu::noipa]]
> +_Bool __GIMPLE (ssa, startwith("forwprop1"))
> +f3 (_Bool a)
> +{
> + _Bool i;
> + _Bool tt;
> +
> + __BB(2):
> + tt_4 = a_1(D) == _Literal (_Bool)0;
> + if (tt_4 != _Literal (_Bool)0)
> + goto __BB3;
> + else
> + goto __BB4;
> +
> + __BB(3):
> + goto __BB5;
> +
> + __BB(4):
> + goto __BB5;
> +
> + __BB(5):
> + i_2 = __PHI (__BB4: a_1(D), __BB3: _Literal (_Bool)0);
> +
> + return i_2;
> +}
> +
> +int main()
> +{
> + if (f3(0))
> + __builtin_abort();
> + if (!f3(1))
> + __builtin_abort();
> +}
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 14aeaadd6f6..2fb28b4e60e 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -726,6 +726,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> gimple *stmt_to_move = NULL;
> gimple *stmt_to_move_alt = NULL;
> auto_bitmap inserted_exprs;
> + tree arg_true, arg_false;
>
> /* Special case A ? B : B as this will always simplify to B. */
> if (operand_equal_for_phi_arg_p (arg0, arg1))
> @@ -756,12 +757,34 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> /* We need to know which is the true edge and which is the false
> edge so that we know when to invert the condition below. */
> extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
> - if (e1 == true_edge || e0 == false_edge)
> - std::swap (arg0, arg1);
> +
> + /* Forward the edges over the middle basic block. */
> + if (true_edge->dest == middle_bb)
> + true_edge = EDGE_SUCC (true_edge->dest, 0);
> + if (false_edge->dest == middle_bb)
> + false_edge = EDGE_SUCC (false_edge->dest, 0);
> +
> + /* When THREEWAY_P then e1 will point to the edge of the final transition
> + from middle-bb to end. */
> + if (true_edge == e0)
> + {
> + if (!threeway_p)
> + gcc_assert (false_edge == e1);
> + arg_true = arg0;
> + arg_false = arg1;
> + }
> + else
> + {
> + gcc_assert (false_edge == e0);
> + if (!threeway_p)
> + gcc_assert (true_edge == e1);
> + arg_true = arg1;
> + arg_false = arg0;
> + }
>
> tree type = TREE_TYPE (gimple_phi_result (phi));
> result = gimple_simplify_phiopt (early_p, type, stmt,
> - arg0, arg1,
> + arg_true, arg_false,
> &seq);
> if (!result)
> return false;
> --
> 2.39.1
>
new file mode 100644
@@ -0,0 +1,40 @@
+/* { dg-do run } */
+/* We need to disable passes which might cause cfg cleanup */
+/* { dg-options "-O1 -fgimple -fdisable-tree-ethread -fdisable-tree-fre1" } */
+
+/* This code is done this way to have the false edge as 1st
+ successor edge of BB2. Normally the true edge would be
+ the first and you would not hit the bug. */
+[[gnu::noipa]]
+_Bool __GIMPLE (ssa, startwith("forwprop1"))
+f3 (_Bool a)
+{
+ _Bool i;
+ _Bool tt;
+
+ __BB(2):
+ tt_4 = a_1(D) == _Literal (_Bool)0;
+ if (tt_4 != _Literal (_Bool)0)
+ goto __BB3;
+ else
+ goto __BB4;
+
+ __BB(3):
+ goto __BB5;
+
+ __BB(4):
+ goto __BB5;
+
+ __BB(5):
+ i_2 = __PHI (__BB4: a_1(D), __BB3: _Literal (_Bool)0);
+
+ return i_2;
+}
+
+int main()
+{
+ if (f3(0))
+ __builtin_abort();
+ if (!f3(1))
+ __builtin_abort();
+}
@@ -726,6 +726,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
gimple *stmt_to_move = NULL;
gimple *stmt_to_move_alt = NULL;
auto_bitmap inserted_exprs;
+ tree arg_true, arg_false;
/* Special case A ? B : B as this will always simplify to B. */
if (operand_equal_for_phi_arg_p (arg0, arg1))
@@ -756,12 +757,34 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
/* We need to know which is the true edge and which is the false
edge so that we know when to invert the condition below. */
extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge);
- if (e1 == true_edge || e0 == false_edge)
- std::swap (arg0, arg1);
+
+ /* Forward the edges over the middle basic block. */
+ if (true_edge->dest == middle_bb)
+ true_edge = EDGE_SUCC (true_edge->dest, 0);
+ if (false_edge->dest == middle_bb)
+ false_edge = EDGE_SUCC (false_edge->dest, 0);
+
+ /* When THREEWAY_P then e1 will point to the edge of the final transition
+ from middle-bb to end. */
+ if (true_edge == e0)
+ {
+ if (!threeway_p)
+ gcc_assert (false_edge == e1);
+ arg_true = arg0;
+ arg_false = arg1;
+ }
+ else
+ {
+ gcc_assert (false_edge == e0);
+ if (!threeway_p)
+ gcc_assert (true_edge == e1);
+ arg_true = arg1;
+ arg_false = arg0;
+ }
tree type = TREE_TYPE (gimple_phi_result (phi));
result = gimple_simplify_phiopt (early_p, type, stmt,
- arg0, arg1,
+ arg_true, arg_false,
&seq);
if (!result)
return false;