Message ID | 20230422220921.452264-3-apinski@marvell.com |
---|---|
State | New |
Headers | show |
Series | Improve PHIOPT match and simplify for diamond shaped bbs | expand |
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 >
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);