| Message ID | 574C09F9.1050201@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 30 May 2016, Tom de Vries wrote: > Hi, > > this patch fixes graphite PR69068, a 6/7 regression. > > > I. > > Consider this test-case: > ... > int qo; > int zh[2]; > > void > td (void) > { > int ly, en; > for (ly = 0; ly < 2; ++ly) > for (en = 0; en < 2; ++en) > zh[en] = ((qo == 0) || (((qo * 2) != 0))) ? 1 : -1; > } > ... > > When compiling with -O1 -fgraphite-identity, we run into an assert in > bb_contains_loop_phi_nodes: > ... > pr-graphite.c: In function ‘td’: > pr-graphite.c:5:1: internal compiler error: in bb_contains_loop_phi_nodes, at > graphite-isl-ast-to-gimple.c:1078 > td(void) > ^~ > ... > > > II. > > At graphite0, we have a 3 argument phi in bb 7: > ... > td () > { > int en; > int ly; > int qo.1_1; > int _3; > int iftmp.0_6; > > <bb 2>: > qo.1_1 = qo; > _3 = qo.1_1 * 2; > goto <bb 10>; > > <bb 3>: > > <bb 4>: > # en_19 = PHI <0(10), en_12(3)> > if (qo.1_1 == 0) > goto <bb 7>; > else > goto <bb 5>; > > <bb 5>: > if (_3 != 0) > goto <bb 6>; > else > goto <bb 7>; > > <bb 6>: > > <bb 7>: > # iftmp.0_6 = PHI <1(6), -1(5), 1(4)> > zh[en_19] = iftmp.0_6; > en_12 = en_19 + 1; > if (en_12 <= 1) > goto <bb 3>; > else > goto <bb 8>; > > <bb 8>: > ly_13 = ly_18 + 1; > if (ly_13 <= 1) > goto <bb 9>; > else > goto <bb 11>; > > <bb 9>: > > <bb 10>: > # ly_18 = PHI <ly_13(9), 0(2)> > goto <bb 4>; > > <bb 11>: > return; > > } > ... > > When instantiating the gimple from the graphite ast, we arrive at > copy_bb_and_scalar_dependences with bb.index == 7, where we execute: > ... > if (num_phis > 0 && bb_contains_loop_phi_nodes (bb)) > { > ... > > Subsequently we run into this assert, because > EDGE_COUNT (bb->preds) == 3: > ... > /* Return true when BB contains loop phi nodes. A loop phi node is the > loop header containing phi nodes which has one init-edge and one > back-edge. */ > > static bool > bb_contains_loop_phi_nodes (basic_block bb) > { > gcc_assert (EDGE_COUNT (bb->preds) <= 2); > ... > > > III. > > This patch fixes the assert conservatively by aborting graphite code > generation when encountering a phi with more than two arguments in > copy_bb_and_scalar_dependences. > > Bootstrapped and reg-tested on x86_64. > > OK for trunk, 6 branch? Did you check if simply returning false from bb_contains_loop_phi_nodes instead of asserting works? The caller has a 'else' that is supposed to handle condition PHIs. After all it already handles one predecessor specially ... Thus if (EDGE_COUNT (bb->preds) != 2) return false; should work here. Or replace this function with bb_loop_header_p (bb) (works w/o loop info, the function seems to depend on loop info and thus simply checking bb->loop_father->header == bb should work as well). Thanks, Richard.
On 30/05/16 11:46, Richard Biener wrote: >> This patch fixes the assert conservatively by aborting graphite code >> >generation when encountering a phi with more than two arguments in >> >copy_bb_and_scalar_dependences. >> > >> >Bootstrapped and reg-tested on x86_64. >> > >> >OK for trunk, 6 branch? > Did you check if simply returning false from bb_contains_loop_phi_nodes > instead of asserting works? The caller has a 'else' that is supposed > to handle condition PHIs. After all it already handles one predecessor > specially ... Thus > > if (EDGE_COUNT (bb->preds) != 2) > return false; > > should work here. Unfortunately, that doesn't work. We run into another assert in copy_cond_phi_nodes: ... /* Cond phi nodes should have exactly two arguments. */ gcc_assert (2 == EDGE_COUNT (bb->preds)); ... > Or replace this function with bb_loop_header_p (bb) > (works w/o loop info, the function seems to depend on loop info and > thus simply checking bb->loop_father->header == bb should work as well). I think that will run into the same assert. Thanks, - Tom
On Mon, 30 May 2016, Tom de Vries wrote: > On 30/05/16 11:46, Richard Biener wrote: > > > This patch fixes the assert conservatively by aborting graphite code > > > >generation when encountering a phi with more than two arguments in > > > >copy_bb_and_scalar_dependences. > > > > > > > >Bootstrapped and reg-tested on x86_64. > > > > > > > >OK for trunk, 6 branch? > > > Did you check if simply returning false from bb_contains_loop_phi_nodes > > instead of asserting works? The caller has a 'else' that is supposed > > to handle condition PHIs. After all it already handles one predecessor > > specially ... Thus > > > > if (EDGE_COUNT (bb->preds) != 2) > > return false; > > > > should work here. > > Unfortunately, that doesn't work. We run into another assert in > copy_cond_phi_nodes: > ... > /* Cond phi nodes should have exactly two arguments. */ > gcc_assert (2 == EDGE_COUNT (bb->preds)); > ... Hah. So can we still do my suggested change and bail out conservatively in Cond PHI node handling instead? Because the PHI node is clearly _not_ a loop header PHI and the cond phi assert is also a latent bug. > > Or replace this function with bb_loop_header_p (bb) > > (works w/o loop info, the function seems to depend on loop info and > > thus simply checking bb->loop_father->header == bb should work as well). > > I think that will run into the same assert. Yes. Richard. > Thanks, > - Tom > > >
Handle 3-arg phi in copy_bb_and_scalar_dependences 2016-05-30 Tom de Vries <tom@codesourcery.com> PR tree-optimization/69068 * graphite-isl-ast-to-gimple.c (copy_bb_and_scalar_dependences): Handle phis with more than two args. * gcc.dg/graphite/pr69068.c: New test. --- gcc/graphite-isl-ast-to-gimple.c | 7 +++++++ gcc/testsuite/gcc.dg/graphite/pr69068.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/gcc/graphite-isl-ast-to-gimple.c b/gcc/graphite-isl-ast-to-gimple.c index ff1d91f..c176db0 100644 --- a/gcc/graphite-isl-ast-to-gimple.c +++ b/gcc/graphite-isl-ast-to-gimple.c @@ -2725,6 +2725,13 @@ copy_bb_and_scalar_dependences (basic_block bb, edge next_e, vec<tree> iv_map) } else { + if (num_phis > 0 + && EDGE_COUNT (bb->preds) > 2) + { + codegen_error = true; + return NULL; + } + new_bb = split_edge (next_e); if (num_phis > 0 && bb_contains_loop_phi_nodes (bb)) { diff --git a/gcc/testsuite/gcc.dg/graphite/pr69068.c b/gcc/testsuite/gcc.dg/graphite/pr69068.c new file mode 100644 index 0000000..0abea06 --- /dev/null +++ b/gcc/testsuite/gcc.dg/graphite/pr69068.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O1 -fgraphite-identity" } */ + +int qo; +int zh[2]; + +void +td (void) +{ + int ly, en; + for (ly = 0; ly < 2; ++ly) + for (en = 0; en < 2; ++en) + zh[en] = ((qo == 0) || (((qo * 2) != 0))) ? 1 : -1; +}
Hi, this patch fixes graphite PR69068, a 6/7 regression. I. Consider this test-case: ... int qo; int zh[2]; void td (void) { int ly, en; for (ly = 0; ly < 2; ++ly) for (en = 0; en < 2; ++en) zh[en] = ((qo == 0) || (((qo * 2) != 0))) ? 1 : -1; } ... When compiling with -O1 -fgraphite-identity, we run into an assert in bb_contains_loop_phi_nodes: ... pr-graphite.c: In function ‘td’: pr-graphite.c:5:1: internal compiler error: in bb_contains_loop_phi_nodes, at graphite-isl-ast-to-gimple.c:1078 td(void) ^~ ... II. At graphite0, we have a 3 argument phi in bb 7: ... td () { int en; int ly; int qo.1_1; int _3; int iftmp.0_6; <bb 2>: qo.1_1 = qo; _3 = qo.1_1 * 2; goto <bb 10>; <bb 3>: <bb 4>: # en_19 = PHI <0(10), en_12(3)> if (qo.1_1 == 0) goto <bb 7>; else goto <bb 5>; <bb 5>: if (_3 != 0) goto <bb 6>; else goto <bb 7>; <bb 6>: <bb 7>: # iftmp.0_6 = PHI <1(6), -1(5), 1(4)> zh[en_19] = iftmp.0_6; en_12 = en_19 + 1; if (en_12 <= 1) goto <bb 3>; else goto <bb 8>; <bb 8>: ly_13 = ly_18 + 1; if (ly_13 <= 1) goto <bb 9>; else goto <bb 11>; <bb 9>: <bb 10>: # ly_18 = PHI <ly_13(9), 0(2)> goto <bb 4>; <bb 11>: return; } ... When instantiating the gimple from the graphite ast, we arrive at copy_bb_and_scalar_dependences with bb.index == 7, where we execute: ... if (num_phis > 0 && bb_contains_loop_phi_nodes (bb)) { ... Subsequently we run into this assert, because EDGE_COUNT (bb->preds) == 3: ... /* Return true when BB contains loop phi nodes. A loop phi node is the loop header containing phi nodes which has one init-edge and one back-edge. */ static bool bb_contains_loop_phi_nodes (basic_block bb) { gcc_assert (EDGE_COUNT (bb->preds) <= 2); ... III. This patch fixes the assert conservatively by aborting graphite code generation when encountering a phi with more than two arguments in copy_bb_and_scalar_dependences. Bootstrapped and reg-tested on x86_64. OK for trunk, 6 branch? Thanks, - Tom