Re: [PATCH 3/3][POPCOUNT] Remove unnecessary if condition in phiopt

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jul 2, 2018 at 3:15 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
>
> Hi Richard,
>
> On 29 June 2018 at 18:45, Richard Biener <richard.guenther@gmail.com> wrote:
> > On Wed, Jun 27, 2018 at 7:09 AM Kugan Vivekanandarajah
> > <kugan.vivekanandarajah@linaro.org> wrote:
> >>
> >> Hi Richard,
> >>
> >> Thanks for the review,
> >>
> >> On 25 June 2018 at 20:20, Richard Biener <richard.guenther@gmail.com> wrote:
> >> > On Fri, Jun 22, 2018 at 11:16 AM Kugan Vivekanandarajah
> >> > <kugan.vivekanandarajah@linaro.org> wrote:
> >> >>
> >> >> gcc/ChangeLog:
> >> >
> >> > @@ -1516,6 +1521,114 @@ minmax_replacement (basic_block cond_bb,
> >> > basic_block middle_bb,
> >> >
> >> >    return true;
> >> >  }
> >> > +/* Convert
> >> > +
> >> > +   <bb 2>
> >> > +   if (b_4(D) != 0)
> >> > +   goto <bb 3>
> >> >
> >> > vertical space before the comment.
> >> Done.
> >>
> >> >
> >> > +                                 edge e0 ATTRIBUTE_UNUSED, edge e1
> >> > ATTRIBUTE_UNUSED,
> >> >
> >> > why pass them if they are unused?
> >> Removed.
> >>
> >> >
> >> > +  if (stmt_count != 2)
> >> > +    return false;
> >> >
> >> > so what about the case when there is no conversion?
> >> Done.
> >>
> >> >
> >> > +  /* Check that we have a popcount builtin.  */
> >> > +  if (!is_gimple_call (popcount)
> >> > +      || !gimple_call_builtin_p (popcount, BUILT_IN_NORMAL))
> >> > +    return false;
> >> > +  tree fndecl = gimple_call_fndecl (popcount);
> >> > +  if ((DECL_FUNCTION_CODE (fndecl) != BUILT_IN_POPCOUNT)
> >> > +      && (DECL_FUNCTION_CODE (fndecl) != BUILT_IN_POPCOUNTL)
> >> > +      && (DECL_FUNCTION_CODE (fndecl) != BUILT_IN_POPCOUNTLL))
> >> > +    return false;
> >> >
> >> > look at popcount handling in tree-vrp.c how to properly also handle
> >> > IFN_POPCOUNT.
> >> > (CASE_CFN_POPCOUNT)
> >> Done.
> >> >
> >> > +  /* Cond_bb has a check for b_4 != 0 before calling the popcount
> >> > +     builtin.  */
> >> > +  if (gimple_code (cond) != GIMPLE_COND
> >> > +      || gimple_cond_code (cond) != NE_EXPR
> >> > +      || TREE_CODE (gimple_cond_lhs (cond)) != SSA_NAME
> >> > +      || rhs != gimple_cond_lhs (cond))
> >> > +    return false;
> >> >
> >> > The check for SSA_NAME is redundant.
> >> > You fail to check that gimple_cond_rhs is zero.
> >> Done.
> >>
> >> >
> >> > +  /* Remove the popcount builtin and cast stmt.  */
> >> > +  gsi = gsi_for_stmt (popcount);
> >> > +  gsi_remove (&gsi, true);
> >> > +  gsi = gsi_for_stmt (cast);
> >> > +  gsi_remove (&gsi, true);
> >> > +
> >> > +  /* And insert the popcount builtin and cast stmt before the cond_bb.  */
> >> > +  gsi = gsi_last_bb (cond_bb);
> >> > +  gsi_insert_before (&gsi, popcount, GSI_NEW_STMT);
> >> > +  gsi_insert_before (&gsi, cast, GSI_NEW_STMT);
> >> >
> >> > use gsi_move_before ().  You need to reset flow sensitive info on the
> >> > LHS of the popcount call as well as on the LHS of the cast.
> >> Done.
> >>
> >> >
> >> > You fail to check the PHI operand on the false edge.  Consider
> >> >
> >> >  if (b != 0)
> >> >    res = __builtin_popcount (b);
> >> >  else
> >> >    res = 1;
> >> >
> >> > You fail to check the PHI operand on the true edge.  Consider
> >> >
> >> >  res = 0;
> >> >  if (b != 0)
> >> >    {
> >> >       __builtin_popcount (b);
> >> >       res = 2;
> >> >    }
> >> >
> >> > and using -fno-tree-dce and whatever you need to keep the
> >> > popcount call in the IL.  A gimple testcase for phiopt will do.
> >> >
> >> > Your testcase relies on popcount detection.  Please write it
> >> > using __builtin_popcount instead.  Write one with a cast and
> >> > one without.
> >> Added the testcases.
> >>
> >> Is this OK now.
> >
> > +  for (gsi = gsi_start_bb (middle_bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > +    {
> >
> > use gsi_after_labels (middle_bb)
> >
> > +  popcount = last_stmt (middle_bb);
> > +  if (popcount == NULL)
> > +    return false;
> >
> > after the counting this test is always false, remove it.
> >
> > +      /* We have a cast stmt feeding popcount builtin.  */
> > +      cast = first_stmt (middle_bb);
> >
> > looking at the implementation of first_stmt this will
> > give you a label in case the BB has one.  I think it's better
> > to merge this and the above with the "counting" like
> >
> > gsi = gsi_start_nondebug_after_labels_bb (middle_bb);
> > if (gsi_end_p (gsi))
> >   return false;
> > cast = gsi_stmt (gsi);
> > gsi_next_nondebug (&gsi);
> > if (!gsi_end_p (gsi))
> >   {
> >     popcount = gsi_stmt (gsi);
> >     gsi_next_nondebug (&gsi);
> >     if (!gsi_end_p (gsi))
> >        return false;
> >   }
> > else
> >   {
> >     popcount = cast;
> >     cast = NULL;
> >   }
>
> Done.
> >
> > +      /* Check that we have a cast prior to that.  */
> > +      if (gimple_code (cast) != GIMPLE_ASSIGN
> > +         || gimple_assign_rhs_code (cast) != NOP_EXPR)
> > +       return false;
> >
> > CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (cast))
> >
> Done.
>
> > +  /* Check PHI arguments.  */
> > +  if (lhs != arg0 || !integer_zerop (arg1))
> > +    return false;
> >
> > that is not sufficient, you do not know whether arg0 is the true
> > value or the false value.  The edge flags will tell you.
>
> Done.
>
> Please find the modified patch. Is this OK. Bootstrapped and
> regression tested with no new regressions.

OK.

Richard.

> Thanks,
> Kugan
> >
> > Otherwise looks OK.
> >
> > Richard.
> >
> >> Thanks,
> >> Kugan
> >> >
> >> > Thanks,
> >> > Richard.
> >> >
> >> >
> >> >> 2018-06-22  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >> >>
> >> >>     * tree-ssa-phiopt.c (cond_removal_in_popcount_pattern): New.
> >> >>     (tree_ssa_phiopt_worker): Call cond_removal_in_popcount_pattern.
> >> >>
> >> >> gcc/testsuite/ChangeLog:
> >> >>
> >> >> 2018-06-22  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >> >>
> >> >>     * gcc.dg/tree-ssa/popcount3.c: New test.