Re: [PATCH] Fix PR tree-optimization/71170

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

On Tue, May 24, 2016 at 5:13 AM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> On 23 May 2016 at 21:35, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Sat, May 21, 2016 at 8:08 AM, Kugan Vivekanandarajah
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>> On 20 May 2016 at 21:07, Richard Biener <richard.guenther@gmail.com> wrote:
>>>> On Fri, May 20, 2016 at 1:51 AM, Kugan Vivekanandarajah
>>>> <kugan.vivekanandarajah@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.
>
> Hi Richard,
>
> Thanks for the review. I also found another issue with this patch.
> I.e. for the stmt_to_insert we will get gimple_bb of NULL which is not
> expected in sort_by_operand_rank. This only showed up only while
> building a version of glibc.
>
> Bootstrap and regression testing are ongoing.Is this OK for trunk if
> passes regression and bootstrap.

Hmm, I'd rather fall thru to the SSA_NAME_VERSION or id comparison here
than to stmt_dominates_stmt which is only well-defined for stmts in the same BB.

So sth like

Index: gcc/tree-ssa-reassoc.c
===================================================================
--- gcc/tree-ssa-reassoc.c      (revision 236630)
+++ gcc/tree-ssa-reassoc.c      (working copy)
@@ -519,6 +519,8 @@ sort_by_operand_rank (const void *pa, co
         See PR60418.  */
       if (!SSA_NAME_IS_DEFAULT_DEF (oea->op)
          && !SSA_NAME_IS_DEFAULT_DEF (oeb->op)
+         && !oea->stmt_to_insert
+         && !oeb->stmt_to_insert
          && SSA_NAME_VERSION (oeb->op) != SSA_NAME_VERSION (oea->op))
        {
          gimple *stmta = SSA_NAME_DEF_STMT (oea->op);

ok with that change.

Richard.

> Thanks,
> Kugan
>
>
> gcc/ChangeLog:
>
> 2016-05-24  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     * tree-ssa-reassoc.c (sort_by_operand_rank): Check for gimple_bb of NULL
>     for stmt_to_insert.
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-05-24  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>     * gcc.dg/tree-ssa/reassoc-44.c: New test.