Re: [PATCH 2/6] PHIOPT: Cleanup tree_ssa_phiopt_worker code

[Richard Biener <richard.guenther@gmail.com>]

On Sun, Apr 23, 2023 at 12:13 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This patch cleans up tree_ssa_phiopt_worker by merging
> common code. Making do_store_elim handled earlier.
> Note this does not change any overall logic of the code,
> just moves code around enough to be able to do this.
> This will make it easier to move code around even more
> and a few other fixes I have.
> Plus I think all of the do_store_elim code really
> should move to its own function as how much code is shared
> is now obvious not much.

Agreed.

> OK? Bootstrapped and tested on x86_64-linux-gnu.

OK.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (tree_ssa_phiopt_worker): Rearrange
>         code for better code readability.
> ---
>  gcc/tree-ssa-phiopt.cc | 212 +++++++++++++++++++++--------------------
>  1 file changed, 107 insertions(+), 105 deletions(-)
>
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 296ba646e62..05f19825ce9 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -175,10 +175,14 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>           std::swap (bb1, bb2);
>           std::swap (e1, e2);
>         }
> -      else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest)
> +      else if (EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest
> +              && single_succ_p (bb2))
>         {
>           diamond_p = true;
>           e2 = EDGE_SUCC (bb2, 0);
> +         /* Make sure bb2 is just a fall through. */
> +         if ((e2->flags & EDGE_FALLTHRU) == 0)
> +           continue;
>         }
>        else
>         continue;
> @@ -190,46 +194,23 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>           || (e1->flags & EDGE_FALLTHRU) == 0)
>         continue;
>
> -      if (do_store_elim && diamond_p)
> +      if (do_store_elim)
>         {
> -         basic_block bb3 = e1->dest;
> -
> -         if (!single_succ_p (bb2)
> -             || (e2->flags & EDGE_FALLTHRU) == 0
> -             || EDGE_COUNT (bb3->preds) != 2)
> -           continue;
> -         if (cond_if_else_store_replacement (bb1, bb2, bb3))
> -           cfgchanged = true;
> -         continue;
> -       }
> -      else if (do_hoist_loads && diamond_p)
> -       {
> -         basic_block bb3 = e1->dest;
> -
> -         if (!FLOAT_TYPE_P (TREE_TYPE (gimple_cond_lhs (cond_stmt)))
> -             && single_succ_p (bb2)
> -             && single_pred_p (bb1)
> -             && single_pred_p (bb2)
> -             && EDGE_COUNT (bb->succs) == 2
> -             && EDGE_COUNT (bb3->preds) == 2
> -             /* If one edge or the other is dominant, a conditional move
> -                is likely to perform worse than the well-predicted branch.  */
> -             && !predictable_edge_p (EDGE_SUCC (bb, 0))
> -             && !predictable_edge_p (EDGE_SUCC (bb, 1)))
> -           hoist_adjacent_loads (bb, bb1, bb2, bb3);
> -         continue;
> -       }
> -
> -      if (diamond_p)
> -       {
> -         if (!single_pred_p (bb1)
> -             || !single_pred_p (bb2)
> -             || !single_succ_p (bb2))
> -           continue;
> -       }
> +         if (diamond_p)
> +           {
> +             basic_block bb3 = e1->dest;
> +
> +             /* Only handle sinking of store from 2 bbs only,
> +                The middle bbs don't need to come from the
> +                if always since we are sinking rather than
> +                hoisting. */
> +             if (EDGE_COUNT (bb3->preds) != 2)
> +               continue;
> +             if (cond_if_else_store_replacement (bb1, bb2, bb3))
> +               cfgchanged = true;
> +             continue;
> +           }
>
> -      if (do_store_elim && !diamond_p)
> -       {
>           /* Also make sure that bb1 only have one predecessor and that it
>              is bb.  */
>           if (!single_pred_p (bb1)
> @@ -243,85 +224,106 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
>             continue;
>           if (cond_store_replacement (bb1, bb2, e1, e2, nontrap))
>             cfgchanged = true;
> +         continue;
>         }
> -      else
> +
> +      if (diamond_p)
>         {
> -         gimple_stmt_iterator gsi;
> -         bool candorest = true;
> +         basic_block bb3 = e1->dest;
> +
> +         if (!single_pred_p (bb1)
> +             || !single_pred_p (bb2))
> +           continue;
>
> -         /* Check that we're looking for nested phis.  */
> -         basic_block merge = diamond_p ? EDGE_SUCC (bb2, 0)->dest : bb2;
> -         gimple_seq phis = phi_nodes (merge);
> +         if (do_hoist_loads
> +             && !FLOAT_TYPE_P (TREE_TYPE (gimple_cond_lhs (cond_stmt)))
> +             && EDGE_COUNT (bb->succs) == 2
> +             && EDGE_COUNT (bb3->preds) == 2
> +             /* If one edge or the other is dominant, a conditional move
> +                is likely to perform worse than the well-predicted branch.  */
> +             && !predictable_edge_p (EDGE_SUCC (bb, 0))
> +             && !predictable_edge_p (EDGE_SUCC (bb, 1)))
> +           {
> +             hoist_adjacent_loads (bb, bb1, bb2, bb3);
> +             continue;
> +           }
> +       }
>
> -         /* Value replacement can work with more than one PHI
> -            so try that first. */
> -         if (!early_p && !diamond_p)
> -           for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
> +      gimple_stmt_iterator gsi;
> +      bool candorest = true;
> +
> +      /* Check that we're looking for nested phis.  */
> +      basic_block merge = diamond_p ? EDGE_SUCC (bb2, 0)->dest : bb2;
> +      gimple_seq phis = phi_nodes (merge);
> +
> +      /* Value replacement can work with more than one PHI
> +        so try that first. */
> +      if (!early_p && !diamond_p)
> +       for (gsi = gsi_start (phis); !gsi_end_p (gsi); gsi_next (&gsi))
> +         {
> +           phi = as_a <gphi *> (gsi_stmt (gsi));
> +           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)
>               {
> -               phi = as_a <gphi *> (gsi_stmt (gsi));
> -               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)
> -                 {
> -                   candorest = false;
> -                   cfgchanged = true;
> -                   break;
> -                 }
> +               candorest = false;
> +               cfgchanged = true;
> +               break;
>               }
> +         }
>
> -         if (!candorest)
> -           continue;
> +      if (!candorest)
> +       continue;
>
> -         phi = single_non_singleton_phi_for_edges (phis, e1, e2);
> -         if (!phi)
> -           continue;
> +      phi = single_non_singleton_phi_for_edges (phis, e1, e2);
> +      if (!phi)
> +       continue;
> +
> +      arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> +      arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
>
> +      /* Something is wrong if we cannot find the arguments in the PHI
> +         node.  */
> +      gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> +
> +      gphi *newphi;
> +      if (single_pred_p (bb1)
> +         && !diamond_p
> +         && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> +                                                         arg0, arg1,
> +                                                         cond_stmt)))
> +       {
> +         phi = newphi;
> +         /* factor_out_conditional_conversion may create a new PHI in
> +            BB2 and eliminate an existing PHI in BB2.  Recompute values
> +            that may be affected by that change.  */
>           arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
>           arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> -
> -         /* Something is wrong if we cannot find the arguments in the PHI
> -            node.  */
>           gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> -
> -         gphi *newphi;
> -         if (single_pred_p (bb1)
> -             && !diamond_p
> -             && (newphi = factor_out_conditional_conversion (e1, e2, phi,
> -                                                             arg0, arg1,
> -                                                             cond_stmt)))
> -           {
> -             phi = newphi;
> -             /* factor_out_conditional_conversion may create a new PHI in
> -                BB2 and eliminate an existing PHI in BB2.  Recompute values
> -                that may be affected by that change.  */
> -             arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> -             arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> -             gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> -           }
> -
> -         /* Do the replacement of conditional if it can be done.  */
> -         if (!early_p
> -             && !diamond_p
> -             && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
> -           cfgchanged = true;
> -         else if (!diamond_p
> -                  && match_simplify_replacement (bb, bb1, e1, e2, phi,
> -                                                 arg0, arg1, early_p))
> -           cfgchanged = true;
> -         else if (!early_p
> -                  && !diamond_p
> -                  && single_pred_p (bb1)
> -                  && cond_removal_in_builtin_zero_pattern (bb, bb1, e1, e2,
> -                                                           phi, arg0, arg1))
> -           cfgchanged = true;
> -         else if (minmax_replacement (bb, bb1, bb2, e1, e2, phi, arg0, arg1,
> -                                      diamond_p))
> -           cfgchanged = true;
> -         else if (single_pred_p (bb1)
> -                  && !diamond_p
> -                  && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> -           cfgchanged = true;
>         }
> +
> +      /* Do the replacement of conditional if it can be done.  */
> +      if (!early_p
> +         && !diamond_p
> +         && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
> +       cfgchanged = true;
> +      else if (!diamond_p
> +              && match_simplify_replacement (bb, bb1, e1, e2, phi,
> +                                             arg0, arg1, early_p))
> +       cfgchanged = true;
> +      else if (!early_p
> +              && !diamond_p
> +              && single_pred_p (bb1)
> +              && cond_removal_in_builtin_zero_pattern (bb, bb1, e1, e2,
> +                                                       phi, arg0, arg1))
> +       cfgchanged = true;
> +      else if (minmax_replacement (bb, bb1, bb2, e1, e2, phi, arg0, arg1,
> +                                  diamond_p))
> +       cfgchanged = true;
> +      else if (single_pred_p (bb1)
> +              && !diamond_p
> +              && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> +       cfgchanged = true;
>      }
>
>    free (bb_order);
> --
> 2.39.1
>