Re: [PR71252][PR71269] Fix trunk errors due to stmt_to_insert

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

On Fri, May 27, 2016 at 2:36 PM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
> On 27 May 2016 at 19:56, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Thu, May 26, 2016 at 11:32 AM, Kugan Vivekanandarajah
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>> Hi Jakub,
>>>
>>>
>>> On 26 May 2016 at 18:18, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> On Thu, May 26, 2016 at 02:17:56PM +1000, Kugan Vivekanandarajah wrote:
>>>>> --- a/gcc/tree-ssa-reassoc.c
>>>>> +++ b/gcc/tree-ssa-reassoc.c
>>>>> @@ -3767,8 +3767,10 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
>>>>>        operand_entry temp = *oe3;
>>>>>        oe3->op = oe1->op;
>>>>>        oe3->rank = oe1->rank;
>>>>> +      oe3->stmt_to_insert = oe1->stmt_to_insert;
>>>>>        oe1->op = temp.op;
>>>>>        oe1->rank= temp.rank;
>>>>> +      oe1->stmt_to_insert = temp.stmt_to_insert;
>>>>
>>>> If you want to swap those 3 fields (what about the others?), can't you write
>>>>       std::swap (oe1->op, oe3->op);
>>>>       std::swap (oe1->rank, oe3->rank);
>>>>       std::swap (oe1->stmt_to_insert, oe3->stmt_to_insert);
>>>> instead and drop operand_entry temp = *oe3; ?
>>>>
>>>>>      }
>>>>>    else if ((oe1->rank == oe3->rank
>>>>>           && oe2->rank != oe3->rank)
>>>>> @@ -3779,8 +3781,10 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
>>>>>        operand_entry temp = *oe2;
>>>>>        oe2->op = oe1->op;
>>>>>        oe2->rank = oe1->rank;
>>>>> +      oe2->stmt_to_insert = oe1->stmt_to_insert;
>>>>>        oe1->op = temp.op;
>>>>>        oe1->rank = temp.rank;
>>>>> +      oe1->stmt_to_insert = temp.stmt_to_insert;
>>>>>      }
>>>>
>>>> Similarly.
>>>
>>> Done. Revised patch attached.
>>
>> Your patch only adds a single testcase, please make sure to include
>> _all_ relevant testcases.
>>
>> The swap should simply swap the whole operand, thus
>>
>>  std::swap (*oe1, *oe3);
>>
>
> Thanks for the review.
>
> I will change this.
>
>> it was probably not updated when all the other fields were added.
>>
>> I don't like the find_insert_point changes or the change before
>
> If we insert the stmt_to_insert before the find_insert_point, we can
> end up inserting stmt_to_insert before its argument defining stmt.
> This can be seen with f951 cp2k_single_file.f90 -O3 -ffast-math
> -march=westmere from PR71252. I am attaching the CFG when all the
> insert_stmt_before_use are moved before.

Hmm, but then this effectively means we should have find_insert_point
for inserting to_insert stmts in the first place?  That is, in
insert_stmt_before_use use find_insert_point on the stmt_to_insert
ops?

> I dont understand Fortran and I am not able to reduce a testcase from this.
>
>> build_and_add_sum.
>> Why not move the if (stmt1) insert; if (stmt2) insert; before the if
>> () unconditionally?
>
> In this case also, we dont know where build_and_add_sum will insert
> the new instruction. It may not be stmts[i] before calling
> build_and_add_sum. Therefore we can end up inserting in a wrong place.
> testcase gfortran.dg/pr71252.f90 would ICE.

So split off build_and_add_sum_1 which does not do stmt insertion and
instead treat it like a to_insert stmt at this point (simply insert it
at stmts[i]
or using find_insert_point)?

>>
>> Do we make progress with just the rest of the changes?  If so please split the
>> patch and include relevant testcases.
>
> I think there are two issues.
> 1. the swap which is obvious
> 2. insertion poing which has some related changes and shows two
> different problems (listed above)
>
> if you prefer, I can send two patches for the above.

Yes please.

> Unfortunately, I am not able to reduce Fortran testcase. Any help from
> anyone is really appreciated.
>
>
> Thanks,
> Kugan
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Kugan