On Sat, May 10, 2014 at 3:53 PM, Marc Glisse <marc.gli...@inria.fr> wrote: > Hello, > > in my recent phiopt patch enhancing value_replacement to optimize > x!=0?x+y:y, I forgot to check that there is no other PHI (not sure how I > managed to miss that since I copy-pasted the line just below the test). > > If there are other phi nodes (with different arguments for those 2 > branches), it would be possible to replace the phi argument and stop there > (as value_replacement does for its other transformation). However, I am > chosing to punt. The cost analysis would be different, and I wrote the > transformation assuming that this single-phi test was already done higher in > the function.
I think we should have some good cost analysis because for this testcase, we should be able to get only one conditional move but right now with punting we don't. > > Bootstrap+testsuite on x86_64-linux-gnu. > > 2014-05-12 Marc Glisse <marc.gli...@inria.fr> > > PR tree-optimization/61140 > gcc/ > * tree-ssa-phiopt.c (value_replacement): Punt on multiple phis. > gcc/testsuite/ > * gcc.dg/tree-ssa/pr61140.c: New file. > > -- > Marc Glisse > Index: gcc/testsuite/gcc.dg/tree-ssa/pr61140.c > =================================================================== > --- gcc/testsuite/gcc.dg/tree-ssa/pr61140.c (revision 0) > +++ gcc/testsuite/gcc.dg/tree-ssa/pr61140.c (working copy) > @@ -0,0 +1,18 @@ > +/* { dg-do run } */ > +/* { dg-options "-O2" } */ > + > +int a[1] = { 1 }, b = 1, c; > + > +int > +main () > +{ > + for (; c < 1; c++) > + if (a[0]) > + { > + a[0] &= 1; > + b = 0; > + } > + if (b) > + __builtin_abort (); > + return 0; > +} > Index: gcc/tree-ssa-phiopt.c > =================================================================== > --- gcc/tree-ssa-phiopt.c (revision 210301) > +++ gcc/tree-ssa-phiopt.c (working copy) > @@ -842,20 +842,24 @@ value_replacement (basic_block cond_bb, > /* Now optimize (x != 0) ? x + y : y to just y. > The following condition is too restrictive, there can easily be > another > stmt in middle_bb, for instance a CONVERT_EXPR for the second > argument. */ > gimple assign = last_and_only_stmt (middle_bb); > if (!assign || gimple_code (assign) != GIMPLE_ASSIGN > || gimple_assign_rhs_class (assign) != GIMPLE_BINARY_RHS > || (!INTEGRAL_TYPE_P (TREE_TYPE (arg0)) > && !POINTER_TYPE_P (TREE_TYPE (arg0)))) > return 0; > > + /* Only transform if it removes the condition. */ > + if (!single_non_singleton_phi_for_edges (phi_nodes (gimple_bb (phi)), e0, > e1)) > + return 0; > + > /* Size-wise, this is always profitable. */ > if (optimize_bb_for_speed_p (cond_bb) > /* The special case is useless if it has a low probability. */ > && profile_status_for_fn (cfun) != PROFILE_ABSENT > && EDGE_PRED (middle_bb, 0)->probability < PROB_EVEN > /* If assign is cheap, there is no point avoiding it. */ > && estimate_num_insns (assign, &eni_time_weights) > >= 3 * estimate_num_insns (cond, &eni_time_weights)) > return 0; > >