On 24 May 2016 at 18:36, Christophe Lyon <christophe.l...@linaro.org> wrote: > On 24 May 2016 at 05:13, Kugan Vivekanandarajah > <kugan.vivekanandara...@linaro.org> wrote: >> 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. >> > > 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 <kug...@linaro.org> * tree-ssa-reassoc.c (sort_by_operand_rank): Check fgimple_bb for NULL. 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];