| Message ID | 56D42341.5090607@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Richard, On 19/04/16 22:11, Richard Biener wrote: > On Tue, Apr 19, 2016 at 1:36 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Tue, Apr 19, 2016 at 1:35 PM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Mon, Feb 29, 2016 at 11:53 AM, kugan >>> <kugan.vivekanandarajah@linaro.org> wrote: >>>>> >>>>> Err. I think the way you implement that in reassoc is ad-hoc and not >>>>> related to reassoc at all. >>>>> >>>>> In fact what reassoc is missing is to handle >>>>> >>>>> -y * z * (-w) * x -> y * x * w * x >>>>> >>>>> thus optimize negates as if they were additional * -1 entries in a >>>>> multiplication chain. And >>>>> then optimize a single remaining * -1 in the result chain to a negate. >>>>> >>>>> Then match.pd handles x + (-y) -> x - y (independent of -frounding-math >>>>> btw). >>>>> >>>>> So no, this isn't ok as-is, IMHO you want to expand the multiplication ops >>>>> chain >>>>> pulling in the * -1 ops (if single-use, of course). >>>>> >>>> >>>> I agree. Here is the updated patch along what you suggested. Does this look >>>> better ? >>> >>> It looks better but I think you want to do factor_out_negate_expr before the >>> first qsort/optimize_ops_list call to catch -1. * z * (-w) which also means you >>> want to simply append a -1. to the ops list rather than adjusting the result >>> with a negate stmt. >>> >>> You also need to guard all this with ! HONOR_SNANS (type) && (! >>> HONOR_SIGNED_ZEROS (type) >>> || ! COMPLEX_FLOAT_TYPE_P (type)) (see match.pd pattern transforming x >>> * -1. to -x). >> >> And please add at least one testcase. > > And it appears to me that you could handle this in linearize_expr_tree > as well, similar > to how we handle MULT_EXPR with acceptable_pow_call there by adding -1. and > op into the ops vec. > I am not sure I understand this. I tried doing this. If I add -1 and rhs1 for the NEGATE_EXPR to ops list, when it come to rewrite_expr_tree constant will be sorted early and would make it hard to generate: x + (-y * z * z) => x - y * z * z Do you want to swap the constant in MULT_EXPR chain (if present) like in swap_ops_for_binary_stmt and then create a NEGATE_EXPR ? Thanks, Kugan > Similar for the x + x + x -> 3 * x case we'd want to add a repeat op when seeing > x + 3 * x + x and use ->count in that patch as well. > > Best split out the > > if (rhscode == MULT_EXPR > && TREE_CODE (binrhs) == SSA_NAME > && acceptable_pow_call (SSA_NAME_DEF_STMT (binrhs), &base, &exponent)) > { > add_repeat_to_ops_vec (ops, base, exponent); > gimple_set_visited (SSA_NAME_DEF_STMT (binrhs), true); > } > else > add_to_ops_vec (ops, binrhs); > > pattern into a helper that handles the other cases. > > Richard. > >> Richard. >> >>> Richard. >>> >>>> Thanks, >>>> Kugan
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c index 17eb64f..bbb5ffb 100644 --- a/gcc/tree-ssa-reassoc.c +++ b/gcc/tree-ssa-reassoc.c @@ -4674,6 +4674,41 @@ attempt_builtin_powi (gimple *stmt, vec<operand_entry *> *ops) return result; } +/* Factor out NEGATE_EXPR from the multiplication operands. */ +static void +factor_out_negate_expr (gimple_stmt_iterator *gsi, + gimple *stmt, vec<operand_entry *> *ops) +{ + operand_entry *oe; + unsigned int i; + int neg_count = 0; + + FOR_EACH_VEC_ELT (*ops, i, oe) + { + if (TREE_CODE (oe->op) != SSA_NAME + || !has_single_use (oe->op)) + continue; + gimple *def_stmt = SSA_NAME_DEF_STMT (oe->op); + if (!is_gimple_assign (def_stmt) + || gimple_assign_rhs_code (def_stmt) != NEGATE_EXPR) + continue; + oe->op = gimple_assign_rhs1 (def_stmt); + neg_count ++; + } + + if (neg_count % 2) + { + tree lhs = gimple_assign_lhs (stmt); + tree tmp = make_temp_ssa_name (TREE_TYPE (lhs), NULL, "reassocneg"); + gimple_set_lhs (stmt, tmp); + gassign *neg_stmt = gimple_build_assign (lhs, NEGATE_EXPR, + tmp); + gimple_set_location (neg_stmt, gimple_location (stmt)); + gimple_set_uid (neg_stmt, gimple_uid (stmt)); + gsi_insert_after (gsi, neg_stmt, GSI_SAME_STMT); + } +} + /* Attempt to optimize CST1 * copysign (CST2, y) -> copysign (CST1 * CST2, y) if CST1 > 0, or CST1 * copysign (CST2, y) -> -copysign (CST1 * CST2, y) if CST1 < 0. */ @@ -4917,6 +4952,12 @@ reassociate_bb (basic_block bb) if (rhs_code == MULT_EXPR) attempt_builtin_copysign (&ops); + if (rhs_code == MULT_EXPR) + { + factor_out_negate_expr (&gsi, stmt, &ops); + ops.qsort (sort_by_operand_rank); + } + if (reassoc_insert_powi_p && rhs_code == MULT_EXPR && flag_unsafe_math_optimizations)