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
