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

[Richard Biener]

On Thu, May 5, 2016 at 2:41 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>>
>> +             int last = ops.length () - 1;
>> +             bool negate_result = false;
>>
>> Do
>>
>>    oe &last = ops.last ();
>>
>
> Done.
>
>>
>> +             if (rhs_code == MULT_EXPR
>> +                 && ops.length () > 1
>> +                 && ((TREE_CODE (ops[last]->op) == INTEGER_CST
>>
>> and last.op here and below
>>
>> +                      && integer_minus_onep (ops[last]->op))
>> +                     || ((TREE_CODE (ops[last]->op) == REAL_CST)
>> +                         && real_equal (&TREE_REAL_CST
>> (ops[last]->op), &dconstm1))))
>>
>
> Done.
>
>> Here the checks !HONOR_SNANS () && (!HONOS_SIGNED_ZEROS ||
>> !COMPLEX_FLOAT_TYPE_P)
>> are missing.  The * -1 might appear literally and you are only allowed
>> to turn it into a negate
>> under the above conditions.
>
>
> Done.
>
>>
>> +               {
>> +                 ops.unordered_remove (last);
>>
>> use ops.pop ();
>>
> Done.
>
>> +                 negate_result = true;
>>
>> Please move the whole thing under the else { } case of the ops.length
>> == 0, ops.length == 1 test chain
>> as you did for the actual emit of the negate.
>>
>
> I see your point. However, when we remove the (-1) from the ops list, that
> intern can result in ops.length becoming 1. Therefore, I moved the  the
> following  if (negate_result), outside the condition.

Ah, indeed.   But now you have to care for ops.length () == 0 and thus
the unconditonally ops.last () may now trap.  So I suggest to
do

+             operand_entry *last;
+             bool negate_result = false;
+             if (rhs_code == MULT_EXPR
+                 && ops.length () > 1
                   && (last = ops.last ())
+                 && ((TREE_CODE (last->op) == INTEGER_CST

to avoid this.

>
>>
>> +                 if (negate_result)
>> +                   {
>> +                     tree tmp = make_ssa_name (TREE_TYPE (lhs));
>> +                     gimple_set_lhs (stmt, tmp);
>> +                     gassign *neg_stmt = gimple_build_assign (lhs,
>> NEGATE_EXPR,
>> +                                                              tmp);
>> +                     gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>> +                     gsi_insert_after (&gsi, neg_stmt, GSI_NEW_STMT);
>> +                     update_stmt (stmt);
>>
>> I think that if powi_result is also built you end up using the wrong
>> stmt so you miss a
>>
>>                          stmt = SSA_NAME_DEF_STMT (lhs);
>
>
> Yes, indeed. This can happen and I have added this.
>
>>
>> here.  Also see the new_lhs handling of the powi_result case - again
>> you need sth
>> similar here (it's handling looks a bit fishy as well - this all needs
>> some comments
>> and possibly a (lot more) testcases).
>>
>> So, please do the above requested changes and verify the 'lhs' issues I
>> pointed
>> out by trying to add a few more testcase that also cover the case where a
>> powi
>> is detected in addition to a negation.  Please also add a testcase that
>> catches
>> (-y) * x * (-z).
>>
>
> Added this to the testcase.
>
> Does this look better now?

Yes - the patch is ok with the above suggested change.

Thanks,
Richard.

>
> Thanks,
> Kugan
>
>
>>> 2016-04-23  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>          PR middle-end/40921
>>>          * gcc.dg/tree-ssa/pr40921.c: New test.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-04-23  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>          PR middle-end/40921
>>>          * tree-ssa-reassoc.c (try_special_add_to_ops): New.
>>>          (linearize_expr_tree): Call try_special_add_to_ops.
>>>          (reassociate_bb): Convert MULT_EXPR by (-1) to NEGATE_EXPR.
>>>
>