On Mon, Jul 2, 2018 at 3:15 AM Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
>
> Hi Richard,
>
> On 29 June 2018 at 18:45, Richard Biener <richard.guent...@gmail.com> wrote:
> > 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;
> > }
>
> 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 <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.
