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
>
