On Thu, May 31, 2018 at 3:51 AM, Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote: > Hi Bin, > > Thanks for the review. Please find the revised patch based on the > review comments. > > Thanks, > Kugan > > On 17 May 2018 at 19:56, Bin.Cheng <amker.ch...@gmail.com> wrote: >> On Thu, May 17, 2018 at 2:39 AM, Kugan Vivekanandarajah >> <kugan.vivekanandara...@linaro.org> wrote: >>> Hi Richard, >>> >>> On 6 March 2018 at 02:24, Richard Biener <richard.guent...@gmail.com> wrote: >>>> On Thu, Feb 8, 2018 at 1:41 AM, Kugan Vivekanandarajah >>>> <kugan.vivekanandara...@linaro.org> wrote: >>>>> Hi Richard, >>>>>
Hi, Thanks very much for working. > +/* Utility function to check if OP is defined by a stmt > + that is a val - 1. If that is the case, set it to STMT. */ > + > +static bool > +ssa_defined_by_and_minus_one_stmt_p (tree op, tree val, gimple **stmt) This is checking if op is defined as val - 1, so name it as ssa_defined_by_minus_one_stmt_p? > +{ > + if (TREE_CODE (op) == SSA_NAME > + && (*stmt = SSA_NAME_DEF_STMT (op)) > + && is_gimple_assign (*stmt) > + && (gimple_assign_rhs_code (*stmt) == PLUS_EXPR) > + && val == gimple_assign_rhs1 (*stmt) > + && integer_minus_onep (gimple_assign_rhs2 (*stmt))) > + return true; > + else > + return false; You can simply return the boolean condition. > +} > + > +/* See if LOOP is a popcout implementation of the form ... > + rhs1 = gimple_assign_rhs1 (and_stmt); > + rhs2 = gimple_assign_rhs2 (and_stmt); > + > + if (ssa_defined_by_and_minus_one_stmt_p (rhs1, rhs2, &and_minus_one)) > + rhs1 = rhs2; > + else if (ssa_defined_by_and_minus_one_stmt_p (rhs2, rhs1, &and_minus_one)) > + ; > + else > + return false; > + > + /* Check the recurrence. */ > + phi = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (and_minus_one)); So gimple_assign_rhs1 (and_minus_one) == rhs1 is always true? Please use rhs1 directly. > + gimple *src_phi = SSA_NAME_DEF_STMT (rhs2); I think this is checking wrong thing and is redundant. Either rhs2 equals to rhs1 or is defined as (rhs1 - 1). For (rhs2 == rhs1) case, the check duplicates checking on phi; for the latter, it's never a PHI stmt and shouldn't be checked. > + if (gimple_code (phi) != GIMPLE_PHI > + || gimple_code (src_phi) != GIMPLE_PHI) > + return false; > + > + dest = gimple_assign_lhs (count_stmt); > + tree fn = builtin_decl_implicit (BUILT_IN_POPCOUNT); > + tree src = gimple_phi_arg_def (src_phi, loop_preheader_edge > (loop)->dest_idx); > + if (adjust) > + iter = fold_build2 (MINUS_EXPR, TREE_TYPE (dest), > + build_call_expr (fn, 1, src), > + build_int_cst (TREE_TYPE (dest), 1)); > + else > + iter = build_call_expr (fn, 1, src); Note tree-ssa-loop-niters.c always use unsigned_type_for (IV-type) as niters type. Though unsigned type is unnecessary in this case, but better to follow existing behavior? > + max = int_cst_value (TYPE_MAX_VALUE (TREE_TYPE (dest))); As richi suggested, max should be the number of bits in type of IV. > + > + niter->assumptions = boolean_false_node; Redundant. > + niter->control.base = NULL_TREE; > + niter->control.step = NULL_TREE; > + niter->control.no_overflow = false; > + niter->niter = iter; > + niter->assumptions = boolean_true_node; > + niter->may_be_zero = boolean_false_node; > + niter->max = max; > + niter->bound = NULL_TREE; > + niter->cmp = ERROR_MARK; > + return true; > +} > + > + Appology if these are nitpickings. Thanks, bin