| Message ID | CAELXzTOEq7_N_ZO2bLot7fSEeqJh86yqxf5suQHpnUk8XW1zug@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. Richard. > > Thanks, > Kugan > > > > On 19 May 2016 at 18:04, Martin Liška <mliska@suse.cz> wrote: >> Hello. >> >> Following patch fixes the ICE as it defensively finds the right >> place where a new STMT should be inserted. >> >> Patch bootstraps on x86_64-linux-gnu and no new regression is introduced. >> >> Ready for trunk? >> Thanks, >> Martin
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
On Thu, May 19, 2016 at 10:26 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: > 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) Btw, it also looks like ops are not sorted after rank: (gdb) p ops.m_vec->m_vecdata[0] $4 = (operand_entry *) 0x27a82e0 (gdb) p ops.m_vec->m_vecdata[1] $5 = (operand_entry *) 0x27a82a0 (gdb) p ops.m_vec->m_vecdata[2] $6 = (operand_entry *) 0x27a8260 (gdb) p ops.m_vec->m_vecdata[3] $7 = (operand_entry *) 0x27a8300 (gdb) p *$4 $8 = {rank = 7, id = 5, op = <ssa_name 0x7ffff6890990>, count = 1} (gdb) p *$5 $9 = {rank = 5, id = 3, op = <ssa_name 0x7ffff6890e58>, count = 1} (gdb) p *$6 $10 = {rank = 7, id = 1, op = <ssa_name 0x7ffff6890900>, count = 1} (gdb) p *$7 $11 = {rank = 7, id = 6, op = <ssa_name 0x7ffff6890948>, count = 1} Richard. > > 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);