diff mbox

Fix PR tree-optimization/71170

Message ID CAELXzTPsEH67qeYf77dLfQYpEFQ=pFA2n+eXQkcB5qCVRfSmnw@mail.gmail.com
State New
Headers show

Commit Message

Kugan Vivekanandarajah May 24, 2016, 8:46 a.m. UTC 
On 24 May 2016 at 18:36, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> 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?


Yes (same patch attached). Also Bootstrap and regression testing on
x86_64-linux-gnu didn’t have no new failures.

Is this OK for trunk?

Thanks,
Kugan

gcc/ChangeLog:

2016-05-24  Kugan Vivekanandarajah  <kuganv@linaro.org>

    * tree-ssa-reassoc.c (sort_by_operand_rank): Check fgimple_bb for NULL.


gcc/testsuite/ChangeLog:

2016-05-24  Kugan Vivekanandarajah  <kuganv@linaro.org>

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

Comments

Jakub Jelinek May 24, 2016, 9:10 a.m. UTC | #1 
On Tue, May 24, 2016 at 06:46:49PM +1000, Kugan Vivekanandarajah wrote:
> 2016-05-24  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
>     * tree-ssa-reassoc.c (sort_by_operand_rank): Check fgimple_bb for NULL.

s/fgimple/gimple/ ?

> --- 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 } */

Why the empty line above?  Either stick there a PR number if one is filed,
or leave it out.

> +/* { 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];

Can bb_rank be ever the same for bbb != bba?  If yes, perhaps it would be
better to fallthrough into the reassoc_stmt_dominates_stmt_p testing
code, if not, perhaps just assert that it is different and just
return the difference unconditionally?

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