[PATCH] PHIOPT: Fix diamond case of match_simplify_replacement

[Richard Biener]

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
>