public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Segher Boessenkool <segher@kernel.crashing.org>
To: Michael Meissner <meissner@linux.vnet.ibm.com>,
	       Florian Weimer <fweimer@gapps.redhat.com>,
	       GCC Patches <gcc-patches@gcc.gnu.org>,
	       David Edelsohn <dje.gcc@gmail.com>,
	       Bill Schmidt <wschmidt@linux.vnet.ibm.com>
Subject: Re: [PATCH] Add attribute((target_clone(...))) to PowerPC
Date: Thu, 01 Jun 2017 20:43:00 -0000	[thread overview]
Message-ID: <20170601204322.GW19687@gate.crashing.org> (raw)
In-Reply-To: <20170531223337.GA13852@ibm-tiger.the-meissners.org>

Hi Mike,

On Wed, May 31, 2017 at 06:33:37PM -0400, Michael Meissner wrote:
> +/* On PowerPC, we have a limited number of target clones that we care about
> +   which means we can use an array to hold the options, rather than having more
> +   elaborate data structures to identify each possible variation.  Order the
> +   clones from the default to the highest ISA.  */
> +const int CLONE_DEFAULT		= 0;	/* default clone.  */
> +const int CLONE_ISA_2_05	= 1;	/* ISA 2.05 (power6).  */
> +const int CLONE_ISA_2_06	= 2;	/* ISA 2.06 (power7).  */
> +const int CLONE_ISA_2_07	= 3;	/* ISA 2.07 (power8).  */
> +const int CLONE_ISA_3_00	= 4;	/* ISA 3.00 (power9).  */
> +const int CLONE_MAX		= 5;

With "you don't have to give the enum a name" I meant write it as

enum {
  CLONE_DEFAULT = 0,
  CLONE_ISA_2_05,
[...]
  CLONE_MASK
};

If you do "const int", I think it should be "static const int"?

> +/* Helper function for printing the function name when debugging.  */
> +
> +static const char *
> +get_decl_name (tree fn)
> +{
> +  tree name;
> +
> +  if (!fn)
> +    return "<null>";
> +
> +  name = DECL_NAME (fn);
> +  if (!name)
> +    return "<no-name>";
> +
> +  return IDENTIFIER_POINTER (name);
> +}

Perhaps this would be useful to have in generic code?

> +rs6000_clone_priority (tree fndecl)
> +{
> +  tree fn_opts = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
> +  HOST_WIDE_INT isa_masks;
> +  int ret = (int) CLONE_DEFAULT;

You don't need this cast afaics.

> +  tree attrs = lookup_attribute ("target", DECL_ATTRIBUTES (fndecl));
> +  const char *attrs_str = NULL;
> +
> +  gcc_assert (attrs != NULL);
> +  attrs = TREE_VALUE (TREE_VALUE (attrs));
> +
> +  gcc_assert (TREE_CODE (attrs) == STRING_CST);
> +  attrs_str = TREE_STRING_POINTER (attrs);

And these asserts neither.  There are more of these: if the code
immediately following an assert will obviously fail (in an obvious way)
if the assert is false, then the assert is just noise, makes reading
the code harder instead of easier.

> +/* This compares the priority of target features in function DECL1 and DECL2.
> +   It returns positive value if DECL1 is higher priority, negative value if
> +   DECL2 is higher priority and 0 if they are the same.  Note, priorities are
> +   ordered from lowest (currently CLONE_ISA_3_0) to highest
> +   (CLONE_DEFAULT).  */

This comment needs updating?  Swap CLONE_ISA_3_0 with CLONE_DEFAULT?

> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
> +  if (targetm.has_ifunc_p ())

Hrm, I still don't see what you need the #ifdef for.  What in the
following code won't compile without it?  Or does targetm.has_ifunc_p
return the wrong answer?

> +    {
> +      struct cgraph_function_version_info *it_v = NULL;
> +      struct cgraph_node *dispatcher_node = NULL;
> +      struct cgraph_function_version_info *dispatcher_version_info = NULL;
> +
> +      /* Right now, the dispatching is done via ifunc.  */
> +      dispatch_decl = make_dispatcher_decl (default_node->decl);
> +
> +      dispatcher_node = cgraph_node::get_create (dispatch_decl);
> +      gcc_assert (dispatcher_node != NULL);
> +      dispatcher_node->dispatcher_function = 1;
> +      dispatcher_version_info
> +	= dispatcher_node->insert_new_function_version ();
> +      dispatcher_version_info->next = default_version_info;
> +      dispatcher_node->definition = 1;
> +
> +      /* Set the dispatcher for all the versions.  */
> +      it_v = default_version_info;
> +      while (it_v != NULL)
> +	{
> +	  it_v->dispatcher_resolver = dispatch_decl;
> +	  it_v = it_v->next;
> +	}
> +    }
> +  else
> +#endif

> +  /* On the PowerPC, we do not need to call __builtin_cpu_init, which is a NOP
> +     on the PowerPC (on the x86_64, it is not a NOP).  The builtin function
> +     __builtin_cpu_support ensures that the TOC fields are setup by requiring a
> +     recent glibc.  If we ever need to call __builtin_cpu_init, we would need
> +     to insert the code here to do the call.  */

Ah cool, thanks :-)


Segher

  parent reply	other threads:[~2017-06-01 20:43 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-25 18:54 Michael Meissner
2017-05-25 20:05 ` Florian Weimer
2017-05-25 20:18   ` Michael Meissner
2017-05-30 22:04     ` Segher Boessenkool
2017-05-31  2:22       ` Michael Meissner
2017-05-31 23:15       ` Michael Meissner
2017-06-01  0:20         ` Michael Meissner
2017-06-01 20:43         ` Segher Boessenkool [this message]
2017-06-02 14:16           ` Michael Meissner
2017-06-02 16:56             ` Segher Boessenkool
2017-06-02 17:39               ` Michael Meissner
2017-06-02 17:41                 ` Segher Boessenkool
2017-06-05 21:20                   ` Michael Meissner

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=20170601204322.GW19687@gate.crashing.org \
    --to=segher@kernel.crashing.org \
    --cc=dje.gcc@gmail.com \
    --cc=fweimer@gapps.redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=meissner@linux.vnet.ibm.com \
    --cc=wschmidt@linux.vnet.ibm.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).