Re: [RFC][PATCH][PR63586] Convert x+x+x+x into 4*x

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

On Sun, Apr 24, 2016 at 12:02 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
> As you have said in the other email, I tried implementing with the
> add_reapeats_to_ops_vec but the whole repeat vector is designed for
> MULT_EXPR chain. I tried changing it but it turned out to be not
> straightforward without lots of re-write. Therefore I tried to implement
> based on your review here. Please tell me what you think.

Hmm, ok.

>>> +/* Transoform repeated addition of same values into multiply with
>>> +   constant.  */
>>>
>>> Transform
>
>
> Done.
>
>>>
>>> +static void
>>> +transform_add_to_multiply (gimple_stmt_iterator *gsi, gimple *stmt,
>>> vec<operand_entry *> *ops)
>>>
>>> split the long line
>
>
> Done.
>
>>>
>>> op_list looks redundant - ops[start]->op gives you the desired value
>>> already and if you
>>> use a vec<std::pair<int, int>> you can have a more C++ish start,end pair.
>>>
>>> +      tree tmp = make_temp_ssa_name (TREE_TYPE (op), NULL,
>>> "reassocmul");
>>> +      gassign *mul_stmt = gimple_build_assign (tmp, MULT_EXPR,
>>> +                                              op, build_int_cst
>>> (TREE_TYPE(op), count));
>>>
>>> this won't work for floating point or complex numbers - you need to use
>>> sth like
>>> fold_convert (TREE_TYPE (op), build_int_cst (integer_type_node, count));
>
>
> Done.
>
>>>
>>> For FP types you need to guard the transform with
>>> flag_unsafe_math_optimizations
>
>
> Done.
>
>>>
>>> +      gimple_set_location (mul_stmt, gimple_location (stmt));
>>> +      gimple_set_uid (mul_stmt, gimple_uid (stmt));
>>> +      gsi_insert_before (gsi, mul_stmt, GSI_SAME_STMT);
>>>
>>> I think you do not want to set the stmt uid
>
>
> assert in reassoc_stmt_dominates_p (gcc_assert (gimple_uid (s1) &&
> gimple_uid (s2))) is failing. So I tried to add the uid of the adjacent stmt
> and it seems to work.

Hmm, yes, other cases seem to do the same.

>>> and you want to insert the
>>> stmt right
>>> after the def of op (or at the original first add - though you can't
>>> get your hands at
>
>
> Done.

maybe instert_stmt_after will help here, I don't think you got the insertion
logic correct, thus insert_stmt_after (mul_stmt, def_stmt) which I think
misses GIMPLE_NOP handling.  At least

+      if (SSA_NAME_VAR (op) != NULL

huh?  I suppose you could have tested SSA_NAME_IS_DEFAULT_DEF
but just the GIMPLE_NOP def-stmt test should be enough.

+         && gimple_code (def_stmt) == GIMPLE_NOP)
+       {
+         gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
+         stmt = gsi_stmt (gsi);
+         gsi_insert_before (&gsi, mul_stmt, GSI_NEW_STMT);

not sure if that is the best insertion point choice, it un-does all
code-sinking done
(and no further sinking is run after the last reassoc pass).  We do know we
are handling all uses of op in our chain so inserting before the plus-expr
chain root should work here (thus 'stmt' in the caller context).  I'd
use that here instead.
I think I'd use that unconditionally even if it works and not bother
finding something
more optimal.

Apart from this this now looks ok to me.

But the testcases need some work


--- a/gcc/testsuite/gcc.dg/tree-ssa/pr63586-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr63586-2.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
...
+
+/* { dg-final { scan-tree-dump-times "\\\*" 4 "reassoc1" } } */

I would have expected 3.  Also please check for \\\* 5 for example
to be more specific (and change the cases so you get different constants
for the different functions).

That said, please make the scans more specific.

Thanks,
Richard.


>>> that easily).  You also don't want to set the location to the last stmt
>>> of the
>>> whole add sequence - simply leave it unset.
>>>
>>> +      oe = operand_entry_pool.allocate ();
>>> +      oe->op = tmp;
>>> +      oe->rank = get_rank (op) * count;
>>>
>>> ?  Why that?  oe->rank should be get_rank (tmp).
>>>
>>> +      oe->id = 0;
>>>
>>> other places use next_operand_entry_id++.  I think you want to simply
>>> use add_to_ops_vec (oe, tmp); here for all of the above.
>
>
> Done.
>
>>>
>>> Please return whether you did any optimization and do the
>>> qsort of the operand vector only if you did sth.
>
>
> Done.
>
>
>>> Testcase with FP math missing.  Likewise with complex or vector math.
>>
>>
>> Btw, does it handle associating
>>
>>    x + 3 * x + x
>>
>> to
>>
>>    5 * x
>>
>> ?
>
>
> Added this to the testcase and verified it is working.
>
> Regression tested and bootstrapped on x86-64-linux-gnu with no new
> regressions.
>
> Is this OK for trunk?
>
> Thanks,
> Kugan
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-04-24  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR middle-end/63586
>         * gcc.dg/tree-ssa/pr63586-2.c: New test.
>         * gcc.dg/tree-ssa/pr63586.c: New test.
>         * gcc.dg/tree-ssa/reassoc-14.c: Adjust multiplication count.
>
> gcc/ChangeLog:
>
> 2016-04-24  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>
>         PR middle-end/63586
>         * tree-ssa-reassoc.c (transform_add_to_multiply): New.
>         (reassociate_bb): Call transform_add_to_multiply.
>
>
>