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

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

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;
  }

+      /* 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))

+  /* 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.

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.