On Thu, Apr 21, 2016 at 1:12 PM, kugan <kugan.vivekanandara...@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.guent...@gmail.com> wrote: >>> >>> On Tue, Apr 19, 2016 at 1:35 PM, Richard Biener >>> <richard.guent...@gmail.com> wrote: >>>> >>>> On Mon, Feb 29, 2016 at 11:53 AM, kugan >>>> <kugan.vivekanandara...@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