From: "Bin.Cheng" <amker.cheng@gmail.com>
To: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Cc: Richard Biener <richard.guenther@gmail.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [RFC][PR82479] missing popcount builtin detection
Date: Fri, 01 Jun 2018 10:06:00 -0000 [thread overview]
Message-ID: <CAHFci28hY5orDvOEkpo=c4ooHq1Z7Vgpnszyuam4g34VEnxvKw@mail.gmail.com> (raw)
In-Reply-To: <CAELXzTM2cqu7XTos=L_A7mW99d+eT6AoOPLVR00B3g7T58zSuA@mail.gmail.com>
On Fri, Jun 1, 2018 at 9:56 AM, Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
> 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?
Is it a simple typo? Because assumptions is set to boolean_true_node
just 5 lines below?
The niters part looks good for me with this change. You may need
richi's approval for other parts?
Thanks,
bin
>
>>> + 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 prev parent reply other threads:[~2018-06-01 10:06 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-24 22:17 Kugan Vivekanandarajah
2018-01-25 10:24 ` Richard Biener
2018-01-31 10:41 ` Kugan Vivekanandarajah
2018-01-31 11:05 ` Richard Biener
2018-02-01 4:08 ` Kugan Vivekanandarajah
2018-02-01 12:21 ` Richard Biener
2018-02-08 0:41 ` Kugan Vivekanandarajah
2018-03-05 15:25 ` Richard Biener
2018-03-06 16:20 ` Bin.Cheng
2018-03-07 8:26 ` Richard Biener
2018-03-07 11:25 ` Bin.Cheng
2018-03-08 22:07 ` Kugan Vivekanandarajah
2018-05-17 2:16 ` Kugan Vivekanandarajah
2018-05-17 10:05 ` Bin.Cheng
2018-05-31 6:52 ` Kugan Vivekanandarajah
2018-05-31 17:53 ` Bin.Cheng
2018-06-01 9:57 ` Kugan Vivekanandarajah
2018-06-01 10:06 ` Bin.Cheng [this message]
2018-06-01 13:12 ` Richard Biener
2018-06-05 9:02 ` Kugan Vivekanandarajah
2018-06-05 11:25 ` Richard Biener
2018-06-07 0:52 ` Kugan Vivekanandarajah
2018-06-07 9:21 ` Richard Biener
2018-06-12 3:10 ` Kugan Vivekanandarajah
2018-06-14 13:57 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHFci28hY5orDvOEkpo=c4ooHq1Z7Vgpnszyuam4g34VEnxvKw@mail.gmail.com' \
--to=amker.cheng@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kugan.vivekanandarajah@linaro.org \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).