Re: [RFC][PATCH][PR40921] Convert x + (-y * z * z) into x - y * z * z

[Richard Biener <richard dot guenther at gmail dot com>]

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