On Thu, May 5, 2016 at 2:41 AM, kugan <kugan.vivekanandara...@linaro.org> wrote: > Hi Richard, > >> >> + int last = ops.length () - 1; >> + bool negate_result = false; >> >> Do >> >> oe &last = ops.last (); >> > > Done. > >> >> + if (rhs_code == MULT_EXPR >> + && ops.length () > 1 >> + && ((TREE_CODE (ops[last]->op) == INTEGER_CST >> >> and last.op here and below >> >> + && integer_minus_onep (ops[last]->op)) >> + || ((TREE_CODE (ops[last]->op) == REAL_CST) >> + && real_equal (&TREE_REAL_CST >> (ops[last]->op), &dconstm1)))) >> > > Done. > >> Here the checks !HONOR_SNANS () && (!HONOS_SIGNED_ZEROS || >> !COMPLEX_FLOAT_TYPE_P) >> are missing. The * -1 might appear literally and you are only allowed >> to turn it into a negate >> under the above conditions. > > > Done. > >> >> + { >> + ops.unordered_remove (last); >> >> use ops.pop (); >> > Done. > >> + negate_result = true; >> >> Please move the whole thing under the else { } case of the ops.length >> == 0, ops.length == 1 test chain >> as you did for the actual emit of the negate. >> > > I see your point. However, when we remove the (-1) from the ops list, that > intern can result in ops.length becoming 1. Therefore, I moved the the > following if (negate_result), outside the condition.
Ah, indeed. But now you have to care for ops.length () == 0 and thus the unconditonally ops.last () may now trap. So I suggest to do + operand_entry *last; + bool negate_result = false; + if (rhs_code == MULT_EXPR + && ops.length () > 1 && (last = ops.last ()) + && ((TREE_CODE (last->op) == INTEGER_CST to avoid this. > >> >> + if (negate_result) >> + { >> + tree tmp = make_ssa_name (TREE_TYPE (lhs)); >> + gimple_set_lhs (stmt, tmp); >> + gassign *neg_stmt = gimple_build_assign (lhs, >> NEGATE_EXPR, >> + tmp); >> + gimple_stmt_iterator gsi = gsi_for_stmt (stmt); >> + gsi_insert_after (&gsi, neg_stmt, GSI_NEW_STMT); >> + update_stmt (stmt); >> >> I think that if powi_result is also built you end up using the wrong >> stmt so you miss a >> >> stmt = SSA_NAME_DEF_STMT (lhs); > > > Yes, indeed. This can happen and I have added this. > >> >> here. Also see the new_lhs handling of the powi_result case - again >> you need sth >> similar here (it's handling looks a bit fishy as well - this all needs >> some comments >> and possibly a (lot more) testcases). >> >> So, please do the above requested changes and verify the 'lhs' issues I >> pointed >> out by trying to add a few more testcase that also cover the case where a >> powi >> is detected in addition to a negation. Please also add a testcase that >> catches >> (-y) * x * (-z). >> > > Added this to the testcase. > > Does this look better now? Yes - the patch is ok with the above suggested change. Thanks, Richard. > > Thanks, > Kugan > > >>> 2016-04-23 Kugan Vivekanandarajah <kug...@linaro.org> >>> >>> PR middle-end/40921 >>> * gcc.dg/tree-ssa/pr40921.c: New test. >>> >>> gcc/ChangeLog: >>> >>> 2016-04-23 Kugan Vivekanandarajah <kug...@linaro.org> >>> >>> PR middle-end/40921 >>> * tree-ssa-reassoc.c (try_special_add_to_ops): New. >>> (linearize_expr_tree): Call try_special_add_to_ops. >>> (reassociate_bb): Convert MULT_EXPR by (-1) to NEGATE_EXPR. >>> >