[PATCH] Replace conditional_replacement with match and simplify
Christophe Lyon
christophe.lyon@linaro.org
Wed Jun 2 08:36:19 GMT 2021
On Tue, 1 Jun 2021 at 08:06, apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> This is the first of series of patches to simplify phi-opt
> to use match and simplify in many cases. This simplification
> will more things to optimize.
>
> This is what Richard requested in
> https://gcc.gnu.org/pipermail/gcc-patches/2021-May/571197.html
> and I think it is the right thing to do too.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.c (match_simplify_replacement):
> New function.
> (tree_ssa_phiopt_worker): Use match_simplify_replacement.
> (two_value_replacement): Change the comment about
> conditional_replacement.
> (conditional_replacement): Delete.
Hi Andrew,
This patch caused a regression on aarch64:
FAIL: gcc.target/aarch64/subs_compare_2.c scan-assembler-not
cmp\\tw[0-9]+, w[0-9]+
FAIL: gcc.target/aarch64/subs_compare_2.c scan-assembler-times
subs\\tw[0-9]+, w[0-9]+, [#]?4 1
Can you check?
Thanks
Christophe
> ---
> gcc/tree-ssa-phiopt.c | 144 ++++++++++++------------------------------
> 1 file changed, 39 insertions(+), 105 deletions(-)
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index e3bd18023a0..969b868397e 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -53,8 +53,8 @@ along with GCC; see the file COPYING3. If not see
> static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
> static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
> tree, tree);
> -static bool conditional_replacement (basic_block, basic_block,
> - edge, edge, gphi *, tree, tree);
> +static bool match_simplify_replacement (basic_block, basic_block,
> + edge, edge, gphi *, tree, tree);
> static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
> gimple *);
> static int value_replacement (basic_block, basic_block,
> @@ -347,8 +347,8 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
> if (!early_p && two_value_replacement (bb, bb1, e2, phi, arg0, arg1))
> cfgchanged = true;
> else if (!early_p
> - && conditional_replacement (bb, bb1, e1, e2, phi,
> - arg0, arg1))
> + && match_simplify_replacement (bb, bb1, e1, e2, phi,
> + arg0, arg1))
> cfgchanged = true;
> else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> cfgchanged = true;
> @@ -675,7 +675,7 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> }
>
> /* Defer boolean x ? 0 : {1,-1} or x ? {1,-1} : 0 to
> - conditional_replacement. */
> + match_simplify_replacement. */
> if (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE
> && (integer_zerop (arg0)
> || integer_zerop (arg1)
> @@ -784,137 +784,71 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> return true;
> }
>
> -/* The function conditional_replacement does the main work of doing the
> - conditional replacement. Return true if the replacement is done.
> +/* The function match_simplify_replacement does the main work of doing the
> + replacement using match and simplify. Return true if the replacement is done.
> Otherwise return false.
> BB is the basic block where the replacement is going to be done on. ARG0
> is argument 0 from PHI. Likewise for ARG1. */
>
> static bool
> -conditional_replacement (basic_block cond_bb, basic_block middle_bb,
> - edge e0, edge e1, gphi *phi,
> - tree arg0, tree arg1)
> +match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> + edge e0, edge e1, gphi *phi,
> + tree arg0, tree arg1)
> {
> - tree result;
> gimple *stmt;
> - gassign *new_stmt;
> tree cond;
> gimple_stmt_iterator gsi;
> edge true_edge, false_edge;
> - tree new_var, new_var2;
> - bool neg = false;
> - int shift = 0;
> - tree nonzero_arg;
> -
> - /* FIXME: Gimplification of complex type is too hard for now. */
> - /* We aren't prepared to handle vectors either (and it is a question
> - if it would be worthwhile anyway). */
> - if (!(INTEGRAL_TYPE_P (TREE_TYPE (arg0))
> - || POINTER_TYPE_P (TREE_TYPE (arg0)))
> - || !(INTEGRAL_TYPE_P (TREE_TYPE (arg1))
> - || POINTER_TYPE_P (TREE_TYPE (arg1))))
> - return false;
> + gimple_seq seq = NULL;
> + tree result;
>
> - /* The PHI arguments have the constants 0 and 1, or 0 and -1 or
> - 0 and (1 << cst), then convert it to the conditional. */
> - if (integer_zerop (arg0))
> - nonzero_arg = arg1;
> - else if (integer_zerop (arg1))
> - nonzero_arg = arg0;
> - else
> - return false;
> - if (integer_pow2p (nonzero_arg))
> - {
> - shift = tree_log2 (nonzero_arg);
> - if (shift && POINTER_TYPE_P (TREE_TYPE (nonzero_arg)))
> - return false;
> - }
> - else if (integer_all_onesp (nonzero_arg))
> - neg = true;
> - else
> + if (!empty_block_p (middle_bb))
> return false;
>
> - if (!empty_block_p (middle_bb))
> + /* Special case A ? B : B as this will always simplify to B. */
> + if (operand_equal_for_phi_arg_p (arg0, arg1))
> return false;
>
> - /* At this point we know we have a GIMPLE_COND with two successors.
> + /* At this point we know we have a GIMPLE_COND with two successors.
> One successor is BB, the other successor is an empty block which
> falls through into BB.
>
> - There is a single PHI node at the join point (BB) and its arguments
> - are constants (0, 1) or (0, -1) or (0, (1 << shift)).
> -
> - So, given the condition COND, and the two PHI arguments, we can
> - rewrite this PHI into non-branching code:
> -
> - dest = (COND) or dest = COND' or dest = (COND) << shift
> + There is a single PHI node at the join point (BB).
>
> - We use the condition as-is if the argument associated with the
> - true edge has the value one or the argument associated with the
> - false edge as the value zero. Note that those conditions are not
> - the same since only one of the outgoing edges from the GIMPLE_COND
> - will directly reach BB and thus be associated with an argument. */
> + So, given the condition COND, and the two PHI arguments, match and simplify
> + can happen on (COND) ? arg0 : arg1. */
>
> stmt = last_stmt (cond_bb);
> - result = PHI_RESULT (phi);
>
> /* To handle special cases like floating point comparison, it is easier and
> less error-prone to build a tree and gimplify it on the fly though it is
> - less efficient. */
> - cond = fold_build2_loc (gimple_location (stmt),
> - gimple_cond_code (stmt), boolean_type_node,
> - gimple_cond_lhs (stmt), gimple_cond_rhs (stmt));
> + less efficient.
> + Don't use fold_build2 here as that might create (bool)a instead of just
> + "a != 0". */
> + cond = build2_loc (gimple_location (stmt),
> + gimple_cond_code (stmt), boolean_type_node,
> + gimple_cond_lhs (stmt), gimple_cond_rhs (stmt));
>
> /* 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 ((e0 == true_edge && integer_zerop (arg0))
> - || (e0 == false_edge && !integer_zerop (arg0))
> - || (e1 == true_edge && integer_zerop (arg1))
> - || (e1 == false_edge && !integer_zerop (arg1)))
> - cond = fold_build1_loc (gimple_location (stmt),
> - TRUTH_NOT_EXPR, TREE_TYPE (cond), cond);
> -
> - if (neg)
> - {
> - cond = fold_convert_loc (gimple_location (stmt),
> - TREE_TYPE (result), cond);
> - cond = fold_build1_loc (gimple_location (stmt),
> - NEGATE_EXPR, TREE_TYPE (cond), cond);
> - }
> - else if (shift)
> - {
> - cond = fold_convert_loc (gimple_location (stmt),
> - TREE_TYPE (result), cond);
> - cond = fold_build2_loc (gimple_location (stmt),
> - LSHIFT_EXPR, TREE_TYPE (cond), cond,
> - build_int_cst (integer_type_node, shift));
> - }
> -
> - /* Insert our new statements at the end of conditional block before the
> - COND_STMT. */
> - gsi = gsi_for_stmt (stmt);
> - new_var = force_gimple_operand_gsi (&gsi, cond, true, NULL, true,
> - GSI_SAME_STMT);
> + if (e1 == true_edge || e0 == false_edge)
> + std::swap (arg0, arg1);
>
> - if (!useless_type_conversion_p (TREE_TYPE (result), TREE_TYPE (new_var)))
> - {
> - location_t locus_0, locus_1;
> + tree type = TREE_TYPE (gimple_phi_result (phi));
> + result = gimple_simplify (COND_EXPR, type,
> + cond,
> + arg0, arg1,
> + &seq, NULL);
> + if (!result)
> + return false;
>
> - new_var2 = make_ssa_name (TREE_TYPE (result));
> - new_stmt = gimple_build_assign (new_var2, CONVERT_EXPR, new_var);
> - gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT);
> - new_var = new_var2;
> + gsi = gsi_last_bb (cond_bb);
>
> - /* Set the locus to the first argument, unless is doesn't have one. */
> - locus_0 = gimple_phi_arg_location (phi, 0);
> - locus_1 = gimple_phi_arg_location (phi, 1);
> - if (locus_0 == UNKNOWN_LOCATION)
> - locus_0 = locus_1;
> - gimple_set_location (new_stmt, locus_0);
> - }
> + if (seq)
> + gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
>
> - replace_phi_edge_with_variable (cond_bb, e1, phi, new_var);
> + replace_phi_edge_with_variable (cond_bb, e1, phi, result);
>
> /* Note that we optimized this PHI. */
> return true;
> @@ -3627,7 +3561,7 @@ gate_hoist_loads (void)
> Conditional Replacement
> -----------------------
>
> - This transformation, implemented in conditional_replacement,
> + This transformation, implemented in match_simplify_replacement,
> replaces
>
> bb0:
> --
> 2.17.1
>
More information about the Gcc-patches
mailing list