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

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

On Sat, Apr 23, 2016 at 3:10 PM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
>
>>> 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").
>
>
> Hi Richard,
>
> Thanks. Here is an attempt which does this.
>
> Regression tested and bootstrapped on x86-64-linux-gnu with no new
> regressions.
>
> Is this OK for trunk?

+             int last = ops.length () - 1;
+             bool negate_result = false;

Do

  oe &last = ops.last ();


+             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))))

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.

+               {
+                 ops.unordered_remove (last);

use ops.pop ();

+                 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.


+                 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);

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).

Otherwise this now looks good.

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.
>