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.
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
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 3b5f36b..52414d3 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -553,12 +553,12 @@ sort_by_operand_rank (const void *pa, const void *pb)
/* Add an operand entry to *OPS for the tree operand OP. */
static void
-add_to_ops_vec (vec<operand_entry *> *ops, tree op)
+add_to_ops_vec (vec<operand_entry *> *ops, tree op, int rank = 0)
{
operand_entry *oe = operand_entry_pool.allocate ();
oe->op = op;
- oe->rank = get_rank (op);
+ oe->rank = rank ? rank : get_rank (op);
oe->id = next_operand_entry_id++;
oe->count = 1;
ops->safe_push (oe);
@@ -1824,7 +1824,7 @@ transform_add_to_multiply (gimple *stmt,
vec<operand_entry *> *ops)
else
insert_stmt_after (mul_stmt, def_stmt);
gimple_set_visited (mul_stmt, true);
- add_to_ops_vec (ops, tmp);
+ add_to_ops_vec (ops, tmp, bb_rank [gimple_bb (stmt)->index]);
changed = true;
}
