On Wed, Jun 27, 2018 at 7:09 AM Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
>
> Hi Richard,
>
> Thanks for the review,
>
> On 25 June 2018 at 20:20, Richard Biener <richard.guent...@gmail.com> wrote:
> > On Fri, Jun 22, 2018 at 11:16 AM Kugan Vivekanandarajah
> > <kugan.vivekanandara...@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 <kug...@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 <kug...@linaro.org>
> >>
> >> * gcc.dg/tree-ssa/popcount3.c: New test.
