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

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

On Tue, Apr 19, 2016 at 1:56 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Mar 2, 2016 at 3:28 PM, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
>> On 29 February 2016 at 05:28, kugan <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>>> That looks better, but I think the unordered_remove will break operand
>>>> sorting
>>>> and thus you probably don't handle x + x + x + x + y + y + y + y + y +
>>>> y + z + z + z + z
>>>> optimally.
>>>>
>>>> I'd say you simply want to avoid the recursion and collect a vector of
>>>> [start, end] pairs
>>>> before doing any modification to the ops vector.
>>>
>>>
>>> Hi Richard,
>>>
>>> Is the attached patch looks better?
>>>
>>
>> Minor comment, I've noticed typos in your updated comment:
>> "There should be two multiplication left in test1 (inculding one generated"
>> should be
>> "There should be two multiplications left in test1 (including one generated"
>
> +/* Transoform repeated addition of same values into multiply with
> +   constant.  */
>
> Transform
>
> +static void
> +transform_add_to_multiply (gimple_stmt_iterator *gsi, gimple *stmt,
> vec<operand_entry *> *ops)
>
> split the long line
>
> 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));
>
> For FP types you need to guard the transform with flag_unsafe_math_optimizations
>
> +      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 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
> 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.
>
> Please return whether you did any optimization and do the
> qsort of the operand vector only if you did sth.
>
> Testcase with FP math missing.  Likewise with complex or vector math.

Btw, does it handle associating

  x + 3 * x + x

to

  5 * x

?

Richard.

> Thanks,
> Richard.
>
>>> Thanks,
>>> Kugan