Re: PR61140: check the phi is unique in value_replacement

[Andrew Pinski <pinskia at gmail dot com>]

On Sat, May 10, 2014 at 3:53 PM, Marc Glisse <marc.glisse@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.glisse@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;
>
>