On Thu, May 31, 2018 at 3:51 AM, Kugan Vivekanandarajah
<[email protected]> 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 <[email protected]> wrote:
>> On Thu, May 17, 2018 at 2:39 AM, Kugan Vivekanandarajah
>> <[email protected]> wrote:
>>> Hi Richard,
>>>
>>> On 6 March 2018 at 02:24, Richard Biener <[email protected]> wrote:
>>>> On Thu, Feb 8, 2018 at 1:41 AM, Kugan Vivekanandarajah
>>>> <[email protected]> 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
