| Message ID | 56D42341.5090607@linaro.org |
|---|---|
| State | New |
| Headers | show |
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). Richard. > Thanks, > Kugan
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. Richard. > Richard. > >> Thanks, >> Kugan
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. 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
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
On Thu, Apr 21, 2016 at 1:12 PM, kugan <kugan.vivekanandarajah@linaro.org> wrote: > 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 ? In addition to linearize_expr handling you need to handle a -1 in the MULT_EXPR chain specially at rewrite_expr_tree time by emitting a NEGATE_EXPR instead of a MULT_EXPR (which also means you should sort the -1 "last"). Richard. > > 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)