diff mbox

Fix PR tree-optimization/71170

Message ID CAELXzTOd0JmSuGLKHohgehvQHsSRNupWG5aA83qrpvrP14P0gQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Kugan Vivekanandarajah May 24, 2016, 3:13 a.m. UTC
On 23 May 2016 at 21:35, Richard Biener <richard.guenther@gmail.com> wrote:
> On Sat, May 21, 2016 at 8:08 AM, Kugan Vivekanandarajah

> <kugan.vivekanandarajah@linaro.org> wrote:

>> On 20 May 2016 at 21:07, Richard Biener <richard.guenther@gmail.com> wrote:

>>> On Fri, May 20, 2016 at 1:51 AM, Kugan Vivekanandarajah

>>> <kugan.vivekanandarajah@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  <kuganv@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  <kuganv@linaro.org>

    * gcc.dg/tree-ssa/reassoc-44.c: New test.

Comments

Christophe Lyon May 24, 2016, 8:36 a.m. UTC | #1
On 24 May 2016 at 05:13, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> On 23 May 2016 at 21:35, Richard Biener <richard.guenther@gmail.com> wrote:

>> On Sat, May 21, 2016 at 8:08 AM, Kugan Vivekanandarajah

>> <kugan.vivekanandarajah@linaro.org> wrote:

>>> On 20 May 2016 at 21:07, Richard Biener <richard.guenther@gmail.com> wrote:

>>>> On Fri, May 20, 2016 at 1:51 AM, Kugan Vivekanandarajah

>>>> <kugan.vivekanandarajah@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.

>


I'm seeing build failures in glibc after you committed r236619.
This new patch is fixing it, right?


> Thanks,

> Kugan

>

>

> gcc/ChangeLog:

>

> 2016-05-24  Kugan Vivekanandarajah  <kuganv@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  <kuganv@linaro.org>

>

>     * gcc.dg/tree-ssa/reassoc-44.c: New test.
diff mbox

Patch

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];