On Sat, May 21, 2016 at 8:08 AM, Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
> On 20 May 2016 at 21:07, Richard Biener <richard.guent...@gmail.com> wrote:
>> On Fri, May 20, 2016 at 1:51 AM, Kugan Vivekanandarajah
>> <kugan.vivekanandara...@linaro.org> wrote:
>>> Hi Richard,
>>>
>>>> I think it should have the same rank as op or op + 1 which is the current
>>>> behavior. Sth else doesn't work correctly here I think, like inserting the
>>>> multiplication not near the definition of op.
>>>>
>>>> Well, the whole "clever insertion" logic is simply flawed.
>>>
>>> What I meant to say was that the simple logic we have now wouldn’t
>>> work. "clever logic" is knowing where exactly where it is needed and
>>> inserting there. I think thats what you are suggesting below in a
>>> simple to implement way.
>>>
>>>> I'd say that ideally we would delay inserting the multiplication to
>>>> rewrite_expr_tree time. For example by adding a ops->stmt_to_insert
>>>> member.
>>>>
>>>
>>> Here is an implementation based on above. Bootstrap on x86-linux-gnu
>>> is OK. regression testing is ongoing.
>>
>> I like it. Please push the insertion code to a helper as I think you need
>> to post-pone setting the stmts UID to that point.
>>
>> Ideally we'd make use of the same machinery in attempt_builtin_powi,
>> removing the special-casing of powi_result. (same as I said that ideally
>> the plus->mult stuff would use the repeat-ops machinery...)
>>
>> I'm not 100% convinced the place you insert the stmt is correct but I
>> haven't spent too much time to decipher reassoc in this area.
>
>
> Hi Richard,
>
> Thanks. Here is a tested version of the patch. I did miss one place
> which I fixed now (tranform_stmt_to_copy) I also created a function to
> do the insertion.
>
>
> Bootstrap and regression testing on x86_64-linux-gnu are fine. Is this
> OK for trunk.
@@ -3798,6 +3805,7 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex,
oe1 = ops[opindex];
oe2 = ops[opindex + 1];
+
if (rhs1 != oe1->op || rhs2 != oe2->op)
{
gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
please remove this stray change.
Ok with that change.
Thanks,
Richard.
> Thanks,
> Kugan
>
>
> gcc/ChangeLog:
>
> 2016-05-21 Kugan Vivekanandarajah <kug...@linaro.org>
>
> PR middle-end/71170
> * tree-ssa-reassoc.c (struct operand_entry): Add field stmt_to_insert.
> (add_to_ops_vec): Add stmt_to_insert.
> (add_repeat_to_ops_vec): Init stmt_to_insert.
> (insert_stmt_before_use): New.
> (transform_add_to_multiply): Remove mult_stmt insertion and add it
> to ops vector.
> (get_ops): Init stmt_to_insert.
> (maybe_optimize_range_tests): Likewise.
> (rewrite_expr_tree): Insert stmt_to_insert before use stmt.
> (rewrite_expr_tree_parallel): Likewise.
> (reassociate_bb): Likewise.
