| Message ID | alpine.DEB.2.10.1406032016520.2149@laptop-mg.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On 06/04/14 01:46, Marc Glisse wrote: > On Tue, 3 Jun 2014, Jeff Law wrote: > >> On 06/03/14 08:08, Richard Biener wrote: >>> 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. > > I was all for propagating, but replace_uses_by calls fold_stmt on the > using statements (that's normal). If I first move the statement and then > replace, I may call fold_stmt on a stmt that isn't dominated by the defs > of all its arguments. If I first replace, the statement I am supposed to > move may have been modified so I need to find out what happened to it. > In the end, the assignment seems easier and safer (and not too bad since > this case almost never happens in practice). > >> 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. > > This one comes from VRP2 apparently. But it isn't the only pass that > produces singleton phis. I didn't look at them closely, but I assume LIM > is normal, DOM maybe less so, etc. DOM & VRP can create them via jump threading. Right now we only fire up the pass_phi_only_cprop after DOM though. DOM runs not too long after LIM and it will propagate away the degenerates as well. One could argue that propagating away the degenerates ought to occur immediately before any phi-opt pass. > > bootstrap+testsuite on x86_64-linux-gnu as usual. > > 2014-06-04 Marc Glisse <marc.glisse@inria.fr> > > PR tree-optimization/61385 > gcc/ > * tree-ssa-phiopt.c (value_replacement): Replace singleton PHIs > with assignments. > gcc/testsuite/ > * gcc.dg/tree-ssa/pr61385.c: New file. Don't you have to verify neither the source nor the destination are marked with SSA_NAME_OCCURS_IN_ABNORMAL_PHI? And if so, you can't propagate away the PHI (which may mean we need to avoid the optimization in that case). I also believe that when turning a PHI into an assignment you have to preserve the parallel nature of the PHI. If the result of one PHI is used as an input in another PHI, then you have to work harder (see the out-of-ssa code). Perhaps this can never be the case with a degenerate. Jeff
On Wed, Jun 4, 2014 at 9:59 AM, Jeff Law <law@redhat.com> wrote: > On 06/04/14 01:46, Marc Glisse wrote: >> >> On Tue, 3 Jun 2014, Jeff Law wrote: >> >>> On 06/03/14 08:08, Richard Biener wrote: >>>> >>>> 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. >> >> >> I was all for propagating, but replace_uses_by calls fold_stmt on the >> using statements (that's normal). If I first move the statement and then >> replace, I may call fold_stmt on a stmt that isn't dominated by the defs >> of all its arguments. If I first replace, the statement I am supposed to >> move may have been modified so I need to find out what happened to it. >> In the end, the assignment seems easier and safer (and not too bad since >> this case almost never happens in practice). >> >>> 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. >> >> >> This one comes from VRP2 apparently. But it isn't the only pass that >> produces singleton phis. I didn't look at them closely, but I assume LIM >> is normal, DOM maybe less so, etc. > > DOM & VRP can create them via jump threading. Right now we only fire up the > pass_phi_only_cprop after DOM though. Right, if we want to get rid of the degenerates early we have to run pass_phi_only_cprop after VRP as well. > DOM runs not too long after LIM and it will propagate away the degenerates > as well. Not sure where LIM would create degenerate PHIs, but I guess you simply see loop-closed PHIs here which are degenerate. > One could argue that propagating away the degenerates ought to occur > immediately before any phi-opt pass. I'd say we should run phi_only_cprop after the first VRPs instead and move the 2nd VRP before the existing phi_only_cprop pass. I'll do the appropriate change. >> >> bootstrap+testsuite on x86_64-linux-gnu as usual. >> >> 2014-06-04 Marc Glisse <marc.glisse@inria.fr> >> >> PR tree-optimization/61385 >> gcc/ >> * tree-ssa-phiopt.c (value_replacement): Replace singleton PHIs >> with assignments. >> gcc/testsuite/ >> * gcc.dg/tree-ssa/pr61385.c: New file. > > Don't you have to verify neither the source nor the destination are marked > with SSA_NAME_OCCURS_IN_ABNORMAL_PHI? And if so, you can't propagate away > the PHI (which may mean we need to avoid the optimization in that case). > > I also believe that when turning a PHI into an assignment you have to > preserve the parallel nature of the PHI. If the result of one PHI is used > as an input in another PHI, then you have to work harder (see the out-of-ssa > code). Perhaps this can never be the case with a degenerate. Hmm, indeed that can happen. So I'd say we should instead simply bail out if the middle-bb has a PHI node. Simple and safe. And with the pass ordering fixes above we shouldn't see degenerates anyway. Richard. > Jeff
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,21 +877,35 @@ 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)))) { + /* Any PHI node is a singleton and we can replace it with an assignment. + We don't just call replace_uses_by because it calls fold_stmt and it + becomes hard to make all the right adjustments in the right order. */ gsi = gsi_for_stmt (cond); + gimple_stmt_iterator psi = gsi_start_phis (middle_bb); + while (!gsi_end_p (psi)) + { + gimple phi_moving = gsi_stmt (psi); + tree phi_res = gimple_phi_result (phi_moving); + tree phi_arg = gimple_phi_arg_def (phi_moving, 0); + gsi_remove (&psi, false); + gsi_insert_before (&gsi, gimple_build_assign (phi_res, phi_arg), + GSI_SAME_STMT); + } + 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; } /* The function minmax_replacement does the main work of doing the minmax