[RFC][PR82479] missing popcount builtin detection
Kugan Vivekanandarajah
kugan.vivekanandarajah@linaro.org
Fri Jun 1 09:57:00 GMT 2018
Hi Bin,
Thanks a lo for the review.
On 1 June 2018 at 03:45, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Thu, May 31, 2018 at 3:51 AM, Kugan Vivekanandarajah
> <kugan.vivekanandarajah@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.cheng@gmail.com> wrote:
>>> On Thu, May 17, 2018 at 2:39 AM, Kugan Vivekanandarajah
>>> <kugan.vivekanandarajah@linaro.org> wrote:
>>>> Hi Richard,
>>>>
>>>> On 6 March 2018 at 02:24, Richard Biener <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Feb 8, 2018 at 1:41 AM, Kugan Vivekanandarajah
>>>>> <kugan.vivekanandarajah@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.
Done.
>
>> +}
>> +
>> +/* 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.
Done.
>> + 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?
Done.
>
>> + 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.
Not sure I understand. If I remove this, I am getting ICE
(segmentation fault). What is the expectation here?
>> + 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 for the review. I am happy to make the changes needed to get it
to how it should be :)
Thanks,
Kugan
>
> Thanks,
> bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-popcount.patch
Type: text/x-patch
Size: 10369 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20180601/daff8192/attachment.bin>
More information about the Gcc-patches
mailing list