public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
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: Thu, 31 May 2018 17:53:00 -0000	[thread overview]
Message-ID: <CAHFci2873=8CTWOW6vwQ4uBBhcUuGq1mrhhFVHgJwT=viHEzcA@mail.gmail.com> (raw)
In-Reply-To: <CAELXzTMZig271TkAKUWsxKkMuU-AW8K+xBqOcDg9VXBJUWm3eA@mail.gmail.com>

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.

> +}
> +
> +/* 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

  reply	other threads:[~2018-05-31 17:45 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 [this message]
2018-06-01  9:57                       ` Kugan Vivekanandarajah
2018-06-01 10:06                         ` Bin.Cheng
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='CAHFci2873=8CTWOW6vwQ4uBBhcUuGq1mrhhFVHgJwT=viHEzcA@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).