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

[Kugan Vivekanandarajah <kugan dot vivekanandarajah at linaro dot org>]

On 19 May 2016 at 18:55, Richard Biener <richard.guenther@gmail.com> wrote:
> On Thu, May 19, 2016 at 10:26 AM, Kugan
> <kugan.vivekanandarajah@linaro.org> wrote:
>> Hi,
>>
>>
>> On 19/05/16 18:21, Richard Biener wrote:
>>> On Thu, May 19, 2016 at 10:12 AM, Kugan Vivekanandarajah
>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>> Hi Martin,
>>>>
>>>> Thanks for the fix. Just to elaborate (as mentioned in PR)
>>>>
>>>> At tree-ssa-reassoc.c:3897, we have:
>>>>
>>>> stmt:
>>>> _15 = _4 + c_7(D);
>>>>
>>>> oe->op def_stmt:
>>>> _17 = c_7(D) * 3;
>>>>
>>>>
>>>> <bb 2>:
>>>> a1_6 = s_5(D) * 2;
>>>> _1 = (long int) a1_6;
>>>> x1_8 = _1 + c_7(D);
>>>> a2_9 = s_5(D) * 4;
>>>> _2 = (long int) a2_9;
>>>> a3_11 = s_5(D) * 6;
>>>> _3 = (long int) a3_11;
>>>> _16 = x1_8 + c_7(D);
>>>> _18 = _1 + _2;
>>>> _4 = _16 + _2;
>>>> _15 = _4 + c_7(D);
>>>> _17 = c_7(D) * 3;
>>>> x_13 = _15 + _3;
>>>> return x_13;
>>>>
>>>>
>>>> The root cause of this the place in which we are adding (_17 = c_7(D)
>>>> * 3). Finding the right place is not always straightforward as this
>>>> case shows.
>>>>
>>>> We could try  Martin LiÅka's approach, We could also move _17 = c_7(D)
>>>> * 3; at tree-ssa-reassoc.c:3897 satisfy the gcc_assert. We could do
>>>> this based on the use count of _17.
>>>>
>>>>
>>>> This patch does this. I have no preferences. Any thoughts ?
>>>
>>> I think the issue may be that you fail to set changed to true for the
>>> degenerate case of ending up with a multiply only.
>>>
>>> Not sure because neither patch contains a testcase.
>>>
>>
>> Sorry, I should have been specific. There is an existing test-case that
>> is failing. Thats why I didn't include a test case.
>>
>> FAIL: gcc.dg/tree-ssa/slsr-30.c (internal compiler error)
>
> Btw, it also looks like ops are not sorted after rank:
>
> (gdb) p ops.m_vec->m_vecdata[0]
> $4 = (operand_entry *) 0x27a82e0
> (gdb) p ops.m_vec->m_vecdata[1]
> $5 = (operand_entry *) 0x27a82a0
> (gdb) p ops.m_vec->m_vecdata[2]
> $6 = (operand_entry *) 0x27a8260
> (gdb) p ops.m_vec->m_vecdata[3]
> $7 = (operand_entry *) 0x27a8300
> (gdb) p *$4
> $8 = {rank = 7, id = 5, op = <ssa_name 0x7ffff6890990>, count = 1}
> (gdb) p *$5
> $9 = {rank = 5, id = 3, op = <ssa_name 0x7ffff6890e58>, count = 1}
> (gdb) p *$6
> $10 = {rank = 7, id = 1, op = <ssa_name 0x7ffff6890900>, count = 1}
> (gdb) p *$7
> $11 = {rank = 7, id = 6, op = <ssa_name 0x7ffff6890948>, count = 1}
>

Hi Richard,

It is sorted but in swap_ops_for_binary_stmt (ops, len - 3, stmt), it
is reordered for some form optimization. Commenting this is not
helping the ICE.

In the ops list, the operand defined by multiplication has to be the
last due to the way we add the multiplication stmt. We donât have the
knowledge to find the optimal point without going through some
complicated logic.

Therefore, I think we either should reorder the stmt (like my previous
patch) Or change the rank of the operand produced by the multiply such
that it will always be the last. This looks like a reasonable think to
do. Please let me know what you think. I am attaching the patch (not
tested). I will do the proper test and report the results if you think
this approach is OK.


Thanks,
Kugan

Attachment: p.txt
Description: Text document