| Message ID | alpine.DEB.2.02.1406031538380.730@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 3, 2014 at 3:48 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > Hello, > > apparently it is possible to have a PHI in the middle basic block of > value_replacement, so I need to move it as well when I move the statement > and remove the block. > > Bootstrap and testsuite on x86_64-linux-gnu (re-running for various reasons > but they had completed successfully yesterday). > > 2014-06-03 Marc Glisse <marc.glisse@inria.fr> > > PR tree-optimization/61385 > gcc/ > * tree-ssa-phiopt.c (value_replacement): Copy PHI nodes before > removing the basic block. > gcc/testsuite/ > * gcc.dg/tree-ssa/pr61385.c: New file. > > -- > Marc Glisse > Index: gcc/testsuite/gcc.dg/tree-ssa/pr61385.c > =================================================================== > --- gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (revision 0) > +++ gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (working copy) > @@ -0,0 +1,43 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +#define assert(x) if (!(x)) __builtin_abort () > + > +int a, b, c, d, e, f, g; > + > +int > +fn1 () > +{ > + int *h = &c; > + for (; c < 1; c++) > + { > + int *i = &a, *k = &a; > + f = 0; > + if (b) > + return 0; > + if (*h) > + { > + int **j = &i; > + *j = 0; > + d = 0; > + } > + else > + g = e = 0; > + if (*h) > + { > + int **l = &k; > + *l = &g; > + } > + d &= *h; > + assert (k == &a || k); > + assert (i); > + } > + return 0; > +} > + > +int > +main () > +{ > + fn1 (); > + return 0; > +} > Index: gcc/tree-ssa-phiopt.c > =================================================================== > --- gcc/tree-ssa-phiopt.c (revision 211178) > +++ gcc/tree-ssa-phiopt.c (working copy) > @@ -877,20 +877,39 @@ value_replacement (basic_block cond_bb, > && operand_equal_for_phi_arg_p (rhs2, cond_lhs) > && neutral_element_p (code_def, cond_rhs, true)) > || (arg1 == rhs2 > && operand_equal_for_phi_arg_p (rhs1, cond_lhs) > && neutral_element_p (code_def, cond_rhs, false)) > || (operand_equal_for_phi_arg_p (arg1, cond_rhs) > && (operand_equal_for_phi_arg_p (rhs2, cond_lhs) > || operand_equal_for_phi_arg_p (rhs1, cond_lhs)) > && absorbing_element_p (code_def, cond_rhs)))) > { > + /* Move potential PHI nodes. */ > + gimple_stmt_iterator psi = gsi_start_phis (middle_bb); > + while (!gsi_end_p (psi)) > + { > + gimple phi_moving = gsi_stmt (psi); > + gimple newphi = create_phi_node (gimple_phi_result (phi_moving), > + cond_bb); > + int nargs = cond_bb->preds->length(); > + location_t loc = gimple_phi_arg_location (phi_moving, 0); > + tree phi_arg = gimple_phi_arg_def (phi_moving, 0); > + for (int i = 0; i < nargs; ++i) > + { > + edge e = (*cond_bb->preds)[i]; > + add_phi_arg (newphi, phi_arg, e, loc); All arguments get the same value (and the PHI in middle-bb is surely a singleton?), so it's way better to re-materialize the PHI as a gimple assignment at the start of the basic block. If they are singletons (which I expect), the easiest way is to propagate their single argument into all uses and simply remove them. Richard. > + } > + update_stmt (newphi); > + gsi_remove (&psi, false); > + } > + > gsi = gsi_for_stmt (cond); > gimple_stmt_iterator gsi_from = gsi_for_stmt (assign); > gsi_move_before (&gsi_from, &gsi); > replace_phi_edge_with_variable (cond_bb, e1, phi, lhs); > return 2; > } > > return 0; > } > >
On Tue, 3 Jun 2014, Richard Biener wrote: > On Tue, Jun 3, 2014 at 3:48 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> Hello, >> >> apparently it is possible to have a PHI in the middle basic block of >> value_replacement, so I need to move it as well when I move the statement >> and remove the block. >> >> Bootstrap and testsuite on x86_64-linux-gnu (re-running for various reasons >> but they had completed successfully yesterday). >> >> 2014-06-03 Marc Glisse <marc.glisse@inria.fr> >> >> PR tree-optimization/61385 >> gcc/ >> * tree-ssa-phiopt.c (value_replacement): Copy PHI nodes before >> removing the basic block. >> gcc/testsuite/ >> * gcc.dg/tree-ssa/pr61385.c: New file. >> >> -- >> Marc Glisse >> Index: gcc/testsuite/gcc.dg/tree-ssa/pr61385.c >> =================================================================== >> --- gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (revision 0) >> +++ gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (working copy) >> @@ -0,0 +1,43 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O2" } */ >> + >> +#define assert(x) if (!(x)) __builtin_abort () >> + >> +int a, b, c, d, e, f, g; >> + >> +int >> +fn1 () >> +{ >> + int *h = &c; >> + for (; c < 1; c++) >> + { >> + int *i = &a, *k = &a; >> + f = 0; >> + if (b) >> + return 0; >> + if (*h) >> + { >> + int **j = &i; >> + *j = 0; >> + d = 0; >> + } >> + else >> + g = e = 0; >> + if (*h) >> + { >> + int **l = &k; >> + *l = &g; >> + } >> + d &= *h; >> + assert (k == &a || k); >> + assert (i); >> + } >> + return 0; >> +} >> + >> +int >> +main () >> +{ >> + fn1 (); >> + return 0; >> +} >> Index: gcc/tree-ssa-phiopt.c >> =================================================================== >> --- gcc/tree-ssa-phiopt.c (revision 211178) >> +++ gcc/tree-ssa-phiopt.c (working copy) >> @@ -877,20 +877,39 @@ value_replacement (basic_block cond_bb, >> && operand_equal_for_phi_arg_p (rhs2, cond_lhs) >> && neutral_element_p (code_def, cond_rhs, true)) >> || (arg1 == rhs2 >> && operand_equal_for_phi_arg_p (rhs1, cond_lhs) >> && neutral_element_p (code_def, cond_rhs, false)) >> || (operand_equal_for_phi_arg_p (arg1, cond_rhs) >> && (operand_equal_for_phi_arg_p (rhs2, cond_lhs) >> || operand_equal_for_phi_arg_p (rhs1, cond_lhs)) >> && absorbing_element_p (code_def, cond_rhs)))) >> { >> + /* Move potential PHI nodes. */ >> + gimple_stmt_iterator psi = gsi_start_phis (middle_bb); >> + while (!gsi_end_p (psi)) >> + { >> + gimple phi_moving = gsi_stmt (psi); >> + gimple newphi = create_phi_node (gimple_phi_result (phi_moving), >> + cond_bb); >> + int nargs = cond_bb->preds->length(); >> + location_t loc = gimple_phi_arg_location (phi_moving, 0); >> + tree phi_arg = gimple_phi_arg_def (phi_moving, 0); >> + for (int i = 0; i < nargs; ++i) >> + { >> + edge e = (*cond_bb->preds)[i]; >> + add_phi_arg (newphi, phi_arg, e, loc); > > All arguments get the same value (and the PHI in middle-bb is surely > a singleton?), Yes, there is a single incoming edge to middle-bb. > so it's way better to re-materialize the PHI as a > gimple assignment at the start of the basic block. I thought there might be a reason why it was a PHI and not an assignment so I wasn't sure doing that would be ok. It is indeed much easier, so I'll do that... > If they are > singletons (which I expect), the easiest way is to propagate their > single argument into all uses and simply remove them. ... or indeed that, now that I have found a function called replace_uses_by which should do exactly what I need. Thanks,
On 06/03/14 08:08, Richard Biener wrote: > On Tue, Jun 3, 2014 at 3:48 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> Hello, >> >> apparently it is possible to have a PHI in the middle basic block of >> value_replacement, so I need to move it as well when I move the statement >> and remove the block. >> >> Bootstrap and testsuite on x86_64-linux-gnu (re-running for various reasons >> but they had completed successfully yesterday). >> >> 2014-06-03 Marc Glisse <marc.glisse@inria.fr> >> >> PR tree-optimization/61385 >> gcc/ >> * tree-ssa-phiopt.c (value_replacement): Copy PHI nodes before >> removing the basic block. >> gcc/testsuite/ >> * gcc.dg/tree-ssa/pr61385.c: New file. >> >> -- >> Marc Glisse >> Index: gcc/testsuite/gcc.dg/tree-ssa/pr61385.c >> =================================================================== >> --- gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (revision 0) >> +++ gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (working copy) >> @@ -0,0 +1,43 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O2" } */ >> + >> +#define assert(x) if (!(x)) __builtin_abort () >> + >> +int a, b, c, d, e, f, g; >> + >> +int >> +fn1 () >> +{ >> + int *h = &c; >> + for (; c < 1; c++) >> + { >> + int *i = &a, *k = &a; >> + f = 0; >> + if (b) >> + return 0; >> + if (*h) >> + { >> + int **j = &i; >> + *j = 0; >> + d = 0; >> + } >> + else >> + g = e = 0; >> + if (*h) >> + { >> + int **l = &k; >> + *l = &g; >> + } >> + d &= *h; >> + assert (k == &a || k); >> + assert (i); >> + } >> + return 0; >> +} >> + >> +int >> +main () >> +{ >> + fn1 (); >> + return 0; >> +} >> Index: gcc/tree-ssa-phiopt.c >> =================================================================== >> --- gcc/tree-ssa-phiopt.c (revision 211178) >> +++ gcc/tree-ssa-phiopt.c (working copy) >> @@ -877,20 +877,39 @@ value_replacement (basic_block cond_bb, >> && operand_equal_for_phi_arg_p (rhs2, cond_lhs) >> && neutral_element_p (code_def, cond_rhs, true)) >> || (arg1 == rhs2 >> && operand_equal_for_phi_arg_p (rhs1, cond_lhs) >> && neutral_element_p (code_def, cond_rhs, false)) >> || (operand_equal_for_phi_arg_p (arg1, cond_rhs) >> && (operand_equal_for_phi_arg_p (rhs2, cond_lhs) >> || operand_equal_for_phi_arg_p (rhs1, cond_lhs)) >> && absorbing_element_p (code_def, cond_rhs)))) >> { >> + /* Move potential PHI nodes. */ >> + gimple_stmt_iterator psi = gsi_start_phis (middle_bb); >> + while (!gsi_end_p (psi)) >> + { >> + gimple phi_moving = gsi_stmt (psi); >> + gimple newphi = create_phi_node (gimple_phi_result (phi_moving), >> + cond_bb); >> + int nargs = cond_bb->preds->length(); >> + location_t loc = gimple_phi_arg_location (phi_moving, 0); >> + tree phi_arg = gimple_phi_arg_def (phi_moving, 0); >> + for (int i = 0; i < nargs; ++i) >> + { >> + edge e = (*cond_bb->preds)[i]; >> + add_phi_arg (newphi, phi_arg, e, loc); > > All arguments get the same value (and the PHI in middle-bb is surely > a singleton?), so it's way better to re-materialize the PHI as a > gimple assignment at the start of the basic block. If they are > singletons (which I expect), the easiest way is to propagate their > single argument into all uses and simply remove them. We certainly want to get rid of them :-) I'd start by finding out which pass left the degenerate, ensure it's not marked as SSA_NAME_OCCURS_IN_ABNORMAL_PHI, then propagate it away. If it's a systemic problem that a particular pass can leave degenerate PHIs, then you might want to schedule the phi-only propagator to run after that pass. It does const/copy propagation seeding from degenerate PHIs, so it ought to be very fast. jeff
On Tue, Jun 3, 2014 at 10:30 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Tue, 3 Jun 2014, Richard Biener wrote: > >> On Tue, Jun 3, 2014 at 3:48 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> >>> Hello, >>> >>> apparently it is possible to have a PHI in the middle basic block of >>> value_replacement, so I need to move it as well when I move the statement >>> and remove the block. >>> >>> Bootstrap and testsuite on x86_64-linux-gnu (re-running for various >>> reasons >>> but they had completed successfully yesterday). >>> >>> 2014-06-03 Marc Glisse <marc.glisse@inria.fr> >>> >>> PR tree-optimization/61385 >>> gcc/ >>> * tree-ssa-phiopt.c (value_replacement): Copy PHI nodes before >>> removing the basic block. >>> gcc/testsuite/ >>> * gcc.dg/tree-ssa/pr61385.c: New file. >>> >>> -- >>> Marc Glisse >>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr61385.c >>> =================================================================== >>> --- gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (revision 0) >>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (working copy) >>> @@ -0,0 +1,43 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-O2" } */ >>> + >>> +#define assert(x) if (!(x)) __builtin_abort () >>> + >>> +int a, b, c, d, e, f, g; >>> + >>> +int >>> +fn1 () >>> +{ >>> + int *h = &c; >>> + for (; c < 1; c++) >>> + { >>> + int *i = &a, *k = &a; >>> + f = 0; >>> + if (b) >>> + return 0; >>> + if (*h) >>> + { >>> + int **j = &i; >>> + *j = 0; >>> + d = 0; >>> + } >>> + else >>> + g = e = 0; >>> + if (*h) >>> + { >>> + int **l = &k; >>> + *l = &g; >>> + } >>> + d &= *h; >>> + assert (k == &a || k); >>> + assert (i); >>> + } >>> + return 0; >>> +} >>> + >>> +int >>> +main () >>> +{ >>> + fn1 (); >>> + return 0; >>> +} >>> Index: gcc/tree-ssa-phiopt.c >>> =================================================================== >>> --- gcc/tree-ssa-phiopt.c (revision 211178) >>> +++ gcc/tree-ssa-phiopt.c (working copy) >>> @@ -877,20 +877,39 @@ value_replacement (basic_block cond_bb, >>> && operand_equal_for_phi_arg_p (rhs2, cond_lhs) >>> && neutral_element_p (code_def, cond_rhs, true)) >>> || (arg1 == rhs2 >>> && operand_equal_for_phi_arg_p (rhs1, cond_lhs) >>> && neutral_element_p (code_def, cond_rhs, false)) >>> || (operand_equal_for_phi_arg_p (arg1, cond_rhs) >>> && (operand_equal_for_phi_arg_p (rhs2, cond_lhs) >>> || operand_equal_for_phi_arg_p (rhs1, cond_lhs)) >>> && absorbing_element_p (code_def, cond_rhs)))) >>> { >>> + /* Move potential PHI nodes. */ >>> + gimple_stmt_iterator psi = gsi_start_phis (middle_bb); >>> + while (!gsi_end_p (psi)) >>> + { >>> + gimple phi_moving = gsi_stmt (psi); >>> + gimple newphi = create_phi_node (gimple_phi_result >>> (phi_moving), >>> + cond_bb); >>> + int nargs = cond_bb->preds->length(); >>> + location_t loc = gimple_phi_arg_location (phi_moving, 0); >>> + tree phi_arg = gimple_phi_arg_def (phi_moving, 0); >>> + for (int i = 0; i < nargs; ++i) >>> + { >>> + edge e = (*cond_bb->preds)[i]; >>> + add_phi_arg (newphi, phi_arg, e, loc); >> >> >> All arguments get the same value (and the PHI in middle-bb is surely >> a singleton?), > > > Yes, there is a single incoming edge to middle-bb. > > >> so it's way better to re-materialize the PHI as a >> gimple assignment at the start of the basic block. > > > I thought there might be a reason why it was a PHI and not an assignment so > I wasn't sure doing that would be ok. It is indeed much easier, so I'll do > that... Maybe it's leftover of loop closed ssa form, but I guess it's useless at the stage of phiopt. Thanks, bin > > >> If they are >> singletons (which I expect), the easiest way is to propagate their >> single argument into all uses and simply remove them. > > > ... or indeed that, now that I have found a function called replace_uses_by > which should do exactly what I need. > > Thanks, > > -- > Marc Glisse
Index: gcc/testsuite/gcc.dg/tree-ssa/pr61385.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (revision 0) +++ gcc/testsuite/gcc.dg/tree-ssa/pr61385.c (working copy) @@ -0,0 +1,43 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +#define assert(x) if (!(x)) __builtin_abort () + +int a, b, c, d, e, f, g; + +int +fn1 () +{ + int *h = &c; + for (; c < 1; c++) + { + int *i = &a, *k = &a; + f = 0; + if (b) + return 0; + if (*h) + { + int **j = &i; + *j = 0; + d = 0; + } + else + g = e = 0; + if (*h) + { + int **l = &k; + *l = &g; + } + d &= *h; + assert (k == &a || k); + assert (i); + } + return 0; +} + +int +main () +{ + fn1 (); + return 0; +} Index: gcc/tree-ssa-phiopt.c =================================================================== --- gcc/tree-ssa-phiopt.c (revision 211178) +++ gcc/tree-ssa-phiopt.c (working copy) @@ -877,20 +877,39 @@ value_replacement (basic_block cond_bb, && operand_equal_for_phi_arg_p (rhs2, cond_lhs) && neutral_element_p (code_def, cond_rhs, true)) || (arg1 == rhs2 && operand_equal_for_phi_arg_p (rhs1, cond_lhs) && neutral_element_p (code_def, cond_rhs, false)) || (operand_equal_for_phi_arg_p (arg1, cond_rhs) && (operand_equal_for_phi_arg_p (rhs2, cond_lhs) || operand_equal_for_phi_arg_p (rhs1, cond_lhs)) && absorbing_element_p (code_def, cond_rhs)))) { + /* Move potential PHI nodes. */ + gimple_stmt_iterator psi = gsi_start_phis (middle_bb); + while (!gsi_end_p (psi)) + { + gimple phi_moving = gsi_stmt (psi); + gimple newphi = create_phi_node (gimple_phi_result (phi_moving), + cond_bb); + int nargs = cond_bb->preds->length(); + location_t loc = gimple_phi_arg_location (phi_moving, 0); + tree phi_arg = gimple_phi_arg_def (phi_moving, 0); + for (int i = 0; i < nargs; ++i) + { + edge e = (*cond_bb->preds)[i]; + add_phi_arg (newphi, phi_arg, e, loc); + } + update_stmt (newphi); + gsi_remove (&psi, false); + } + gsi = gsi_for_stmt (cond); gimple_stmt_iterator gsi_from = gsi_for_stmt (assign); gsi_move_before (&gsi_from, &gsi); replace_phi_edge_with_variable (cond_bb, e1, phi, lhs); return 2; } return 0; }