| Message ID | CAELXzTOEq7_N_ZO2bLot7fSEeqJh86yqxf5suQHpnUk8XW1zug@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, On 19/05/16 18:21, Richard Biener wrote: > On Thu, May 19, 2016 at 10:12 AM, Kugan Vivekanandarajah > <kugan.vivekanandarajah@linaro.org> wrote: >> Hi Martin, >> >> Thanks for the fix. Just to elaborate (as mentioned in PR) >> >> At tree-ssa-reassoc.c:3897, we have: >> >> stmt: >> _15 = _4 + c_7(D); >> >> oe->op def_stmt: >> _17 = c_7(D) * 3; >> >> >> <bb 2>: >> a1_6 = s_5(D) * 2; >> _1 = (long int) a1_6; >> x1_8 = _1 + c_7(D); >> a2_9 = s_5(D) * 4; >> _2 = (long int) a2_9; >> a3_11 = s_5(D) * 6; >> _3 = (long int) a3_11; >> _16 = x1_8 + c_7(D); >> _18 = _1 + _2; >> _4 = _16 + _2; >> _15 = _4 + c_7(D); >> _17 = c_7(D) * 3; >> x_13 = _15 + _3; >> return x_13; >> >> >> The root cause of this the place in which we are adding (_17 = c_7(D) >> * 3). Finding the right place is not always straightforward as this >> case shows. >> >> We could try Martin Liška's approach, We could also move _17 = c_7(D) >> * 3; at tree-ssa-reassoc.c:3897 satisfy the gcc_assert. We could do >> this based on the use count of _17. >> >> >> This patch does this. I have no preferences. Any thoughts ? > > I think the issue may be that you fail to set changed to true for the > degenerate case of ending up with a multiply only. > > Not sure because neither patch contains a testcase. > Sorry, I should have been specific. There is an existing test-case that is failing. Thats why I didn't include a test case. FAIL: gcc.dg/tree-ssa/slsr-30.c (internal compiler error) Thanks, Kugan
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c index 3b5f36b..11beb28 100644 --- a/gcc/tree-ssa-reassoc.c +++ b/gcc/tree-ssa-reassoc.c @@ -3830,6 +3830,29 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex, } else { + /* Change the position of newly added stmt. */ + if (TREE_CODE (oe1->op) == SSA_NAME + && has_zero_uses (oe1->op) + && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (oe1->op))) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (oe1->op); + gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); + gsi_remove (&gsi, true); + gsi = gsi_for_stmt (stmt); + gsi_insert_before (&gsi, def_stmt, GSI_SAME_STMT); + } + + /* Change the position of newly added stmt. */ + if (TREE_CODE (oe2->op) == SSA_NAME + && has_zero_uses (oe2->op) + && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (oe2->op))) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (oe2->op); + gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); + gsi_remove (&gsi, true); + gsi = gsi_for_stmt (stmt); + gsi_insert_before (&gsi, def_stmt, GSI_SAME_STMT); + } gcc_checking_assert (find_insert_point (stmt, oe1->op, oe2->op) == stmt); gimple_assign_set_rhs1 (stmt, oe1->op); @@ -3894,6 +3917,17 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex, } else { + /* Change the position of newly added stmt. */ + if (TREE_CODE (oe->op) == SSA_NAME + && has_zero_uses (oe->op) + && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (oe->op))) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (oe->op); + gimple_stmt_iterator gsi = gsi_for_stmt (def_stmt); + gsi_remove (&gsi, true); + gsi = gsi_for_stmt (stmt); + gsi_insert_before (&gsi, def_stmt, GSI_SAME_STMT); + } gcc_checking_assert (find_insert_point (stmt, new_rhs1, oe->op) == stmt); gimple_assign_set_rhs1 (stmt, new_rhs1);