On 23 May 2016 at 21:35, Richard Biener <richard.guent...@gmail.com> wrote:
> On Sat, May 21, 2016 at 8:08 AM, Kugan Vivekanandarajah
> <kugan.vivekanandara...@linaro.org> wrote:
>> On 20 May 2016 at 21:07, Richard Biener <richard.guent...@gmail.com> wrote:
>>> On Fri, May 20, 2016 at 1:51 AM, Kugan Vivekanandarajah
>>> <kugan.vivekanandara...@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.
Thanks,
Kugan
gcc/ChangeLog:
2016-05-24 Kugan Vivekanandarajah <kug...@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 <kug...@linaro.org>
* gcc.dg/tree-ssa/reassoc-44.c: New test.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c
b/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c
index e69de29..9b12212 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c
@@ -0,0 +1,10 @@
+
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+unsigned int a;
+int b, c;
+void fn1 ()
+{
+ b = a + c + c;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index fb683ad..06f4d1b 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -525,7 +525,7 @@ sort_by_operand_rank (const void *pa, const void *pb)
gimple *stmtb = SSA_NAME_DEF_STMT (oeb->op);
basic_block bba = gimple_bb (stmta);
basic_block bbb = gimple_bb (stmtb);
- if (bbb != bba)
+ if (bba && bbb && bbb != bba)
{
if (bb_rank[bbb->index] != bb_rank[bba->index])
return bb_rank[bbb->index] - bb_rank[bba->index];
