On Thu, May 19, 2016 at 2:18 PM, Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
> On 19 May 2016 at 18:55, Richard Biener <richard.guent...@gmail.com> wrote:
>> On Thu, May 19, 2016 at 10:26 AM, Kugan
>> <kugan.vivekanandara...@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.vivekanandara...@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.
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.
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.
Richard.
> 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
