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? > 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.