On Thu, Jul 16, 2015 at 04:20:59PM +0100, Kyrill Tkachov wrote:
> Hi all,
>
> This patch implements target attribute support via the
> TARGET_OPTION_VALID_ATTRIBUTE_P hook.
> The aarch64_handle_option function in common/config/aarch64/aarch64-common.c
> is exported to the
> backend and beefed up a bit.
>
> The target attributes supported by this patch reflect the command-line
> options that we specified as Save
> earlier in the series. Explicitly, the target attributes supported are:
> - "general-regs-only"
> - "fix-cortex-a53-835769" and "no-fix-cortex-a53-835769"
> - "cmodel="
> - "strict-align"
> - "omit-leaf-frame-pointer" and "no-omit-leaf-frame-pointer"
> - "tls-dialect"
> - "arch="
> - "cpu="
> - "tune="
>
> These correspond to equivalent command-line options when prefixed with a '-m'.
> Additionally, this patch supports specifying architectural features, as in
> the -march and -mcpu options
> by themselves. So, for example we can write:
> __attribute__((target("+simd+crypto")))
> to enable 'simd' and 'crypto' on a per-function basis.
>
> The documentation and tests for this come as a separate patch later after the
> target pragma support and
> the inlining rules are implemented.
>
> Bootstrapped and tested on aarch64.
>
In addition to the comments below, you may want to run
contrib/check_GNU_style.sh on this patch, it shows a number of other
issues that would be nice to fix.
<...>
> + case AARCH64_PARSE_MISSING_ARG:
> + error ("missing arch name in target %s arch=%qs", pragma_or_attr, str);
This gives the string:
missing arch name in target attribute arch=<string>
How about:
missing architecture name in 'arch' target attribute
> + break;
> + case AARCH64_PARSE_INVALID_ARG:
> + error ("unknown value %qs for target %s arch=", str, pragma_or_attr);
This gives the string:
"unknown value <string> for target attribute arch="
How about:
"unknown value <string> for 'arch' target attribute"
> + switch (parse_res)
> + {
> + case AARCH64_PARSE_MISSING_ARG:
> + error ("missing cpu name in target %s cpu=%qs", pragma_or_attr, str);
> + break;
As above, we can make this more clear.
> + case AARCH64_PARSE_INVALID_ARG:
> + error ("unknown value %qs for target %s cpu", pragma_or_attr, str);
Here you have the arguments backwards and are inconsistent with the error
message above.
> + break;
> + case AARCH64_PARSE_INVALID_FEATURE:
> + error ("invalid feature modifier in target %s cpu=%qs",
> + pragma_or_attr, str);
> + break;
> + default:
> + gcc_unreachable ();
> + }
> +
> + return false;
> +}
> +
> +/* Handle the argument STR to the tune= target attribute.
> + PRAGMA_OR_ATTR is used in potential error messages. */
> +
> +static bool
> +aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr)
> +{
> + const struct processor *tmp_tune = NULL;
> + enum aarch64_parse_opt_result parse_res
> + = aarch64_parse_tune (str, &tmp_tune);
> +
> + if (parse_res == AARCH64_PARSE_OK)
> + {
> + gcc_assert (tmp_tune);
> + selected_tune = tmp_tune;
> + explicit_tune_core = selected_tune->ident;
> + return true;
> + }
> +
> + switch (parse_res)
> + {
> + case AARCH64_PARSE_INVALID_ARG:
> + error ("unknown value %qs for target %s tune=", pragma_or_attr, str);
Again, the arguments are backwards, this will say:
"unknown value attribute for target <string> tune="
> + break;
> + default:
> + gcc_unreachable ();
> + }
> +
> + return false;
> +}
> +
> +/* Parse an isa extensions target attribute string specified in STR.
Two spaces after isa, and capitalise or spell out the meaning of ISA
(preferably avoid the acronym if you can).
> + For example "+fp+nosimd". Show any errors if needed and return true
> + if successful. Update aarch64_isa_flags to reflect the ISA features
Show any errors if needed. Return TRUE if successful.
> + modified.
> + PRAGMA_OR_ATTR is used in potential error messages. */
> +
> +static bool
> +aarch64_handle_attr_isa_flags (char *str, const char *pragma_or_attr)
> +{
> + enum aarch64_parse_opt_result parse_res;
> + unsigned long isa_flags = aarch64_isa_flags;
> +
> + parse_res = aarch64_parse_extension (str, &isa_flags);
> +
> + if (parse_res == AARCH64_PARSE_OK)
> + {
> + aarch64_isa_flags = isa_flags;
> + return true;
> + }
> +
> + switch (parse_res)
> + {
> + case AARCH64_PARSE_MISSING_ARG:
> + error ("missing feature modifier in target %s %qs",
> + pragma_or_attr, str);
> + break;
> +
> + case AARCH64_PARSE_INVALID_FEATURE:
> + error ("invalid feature modifier in target %s %qs",
> + pragma_or_attr, str);
> + break;
> +
> + default:
> + gcc_unreachable ();
> + }
> +
> + return false;
> +}
> +
> +/* The target attributes that we support. On top of these we also support
> just
> + ISA extensions, like __attribute__((target("+crc"))), but that case is
> + handled explicitly in aarch64_process_one_target_attr. */
Space here.
> +static const struct aarch64_attribute_info aarch64_attributes[] =
> +{
> + { "general-regs-only", aarch64_attr_mask, false, NULL,
> OPT_mgeneral_regs_only },
> + { "fix-cortex-a53-835769", aarch64_attr_bool, true, NULL,
> OPT_mfix_cortex_a53_835769 },
> + { "cmodel", aarch64_attr_enum, false, NULL,
> OPT_mcmodel_ },
> + { "strict-align", aarch64_attr_mask, false, NULL,
> OPT_mstrict_align },
> + { "omit-leaf-frame-pointer", aarch64_attr_bool, true, NULL,
> OPT_momit_leaf_frame_pointer },
> + { "tls-dialect", aarch64_attr_enum, false, NULL,
> OPT_mtls_dialect_ },
> + { "arch", aarch64_attr_custom, false,
> aarch64_handle_attr_arch, OPT_march_ },
> + { "cpu", aarch64_attr_custom, false,
> aarch64_handle_attr_cpu, OPT_mcpu_ },
> + { "tune", aarch64_attr_custom, false,
> aarch64_handle_attr_tune, OPT_mtune_ },
> + { NULL, aarch64_attr_custom, false, NULL, OPT____ }
> +};
> +
> +#define SKIP_WHITE(P) do { while (*P == ' ' || *P == '\t') P++; } while (0)
Not sure this really needs to be factored out (and especially not as a
macro!), SKIP_WHITESPACE would be a better name if you have to do it
this way.
> +
> +/* Parse ARG_STR which contains the definition of one target attribute.
> + Show appropriate errors if any or return true if the attribute is valid.
> + PRAGMA_OR_ATTR holds the string to use in error messages about whether
> + we're processing a target attribute or pragma. */
> +
> +static bool
> +aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
> +{
> + bool ret;
> + bool invert = false;
> +
> + int len = strlen (arg_str);
> +
> + if (len == 0)
> + {
> + error ("malformed target %s", pragma_or_attr);
> + return false;
> + }
> +
> + char *str_to_check = (char *) alloca (len + 1);
Seems to go against your approach earlier in the patch series of XSTRDUP,
it would be nice to stay consistent.
> + strcpy (str_to_check, arg_str);
> +
> + SKIP_WHITE (str_to_check);
> +
> + /* We have something like __attribute__((target("+fp+nosimd"))).
> + It is easier to detect and handle it explicitly here rather than going
> + through the machinery for the rest of the target attributes in this
> + function. */
> + if (*str_to_check == '+')
> + return aarch64_handle_attr_isa_flags (str_to_check, pragma_or_attr);
> +
> + if (len > 3 && strncmp (str_to_check, "no-", 3) == 0)
> + {
> + invert = true;
> + str_to_check += 3;
> + }
> + char *arg = strchr (str_to_check, '=');
> +
> + /* If we found opt=foo then terminate STR_TO_CHECK at the '='
> + and point ARG to "foo". */
> + if (arg)
> + {
> + *arg = '\0';
> + arg++;
> + }
> + const struct aarch64_attribute_info *p_attr;
> + ret = false;
> + for (p_attr = aarch64_attributes; !ret && p_attr->name; p_attr++)
> + {
> + /* If the names don't match up, or the user has given an argument
> + to an attribute that doesn't accept one, or didn't give an argument
> + to one that expects, fail to match. */
[...] didn't give an argument to an attribute that expects one [...]
> + if (strcmp (str_to_check, p_attr->name) != 0)
> + continue;
> +
> + bool attr_need_arg_p = p_attr->attr_type == aarch64_attr_custom
> + || p_attr->attr_type == aarch64_attr_enum;
> +
> + bool attr_user_arg_p = arg != NULL;
Do we need this extra variable, seems a little redundant.
> + /* If the user has given an argument to an attribute that doesn't
> + accept one, or didn't give an argument to one that expects,
> + fail to match. */
Copy and paste comment from above.
> + if (attr_need_arg_p ^ attr_user_arg_p)
> + {
> + error ("target %s %qs does not accept an argument",
> + pragma_or_attr, str_to_check);
> + continue;
> + }
> +
> + /* If the name matches but the attribute does not allow "no-" versions
> + then we can't match. */
> + if (invert && !p_attr->allow_neg)
> + {
> + error ("target %s %qs does not allow a negated form",
> + pragma_or_attr, str_to_check);
> + continue;
For all the difference it makes... Why continue and not just return
early?
> + }
> +
> + switch (p_attr->attr_type)
> + {
> + /* Has a custom handler registered.
> + For example, cpu=, arch=, tune=. */
> + case aarch64_attr_custom:
> + gcc_assert (p_attr->handler);
> + gcc_assert (arg);
Useless assert as you've just validated this above.
> + ret = p_attr->handler (arg, pragma_or_attr);
> + break;
> +
> + /* Either set or unset a boolean option. */
> + case aarch64_attr_bool:
> + {
> + struct cl_decoded_option decoded;
> +
> + generate_option (p_attr->opt_num, NULL, !invert,
> + CL_TARGET, &decoded);
> + aarch64_handle_option (&global_options, &global_options_set,
> + &decoded, input_location);
> + ret = true;
> + break;
> + }
> + /* Set or unset a bit in the target_flags. aarch64_handle_option
> + should know what mask to apply given the option number. */
> + case aarch64_attr_mask:
> + {
> + struct cl_decoded_option decoded;
> + /* We only need to specify the option number.
> + aarch64_handle_option will know which mask to apply. */
> + decoded.opt_index = p_attr->opt_num;
> + decoded.value = !invert;
> + aarch64_handle_option (&global_options, &global_options_set,
> + &decoded, input_location);
> + ret = true;
> + break;
> + }
> + /* Use the option setting machinery to set an option to an enum. */
> + case aarch64_attr_enum:
> + {
> + gcc_assert (arg);
> + bool valid;
> + int value;
> + valid = opt_enum_arg_to_value (p_attr->opt_num, arg,
> + &value, CL_TARGET);
> + if (valid)
> + {
> + set_option (&global_options, NULL, p_attr->opt_num, value,
> + NULL, DK_UNSPECIFIED, input_location,
> + global_dc);
> + ret = true;
> + }
> + else
> + {
> + error ("target %s %s=%s is not valid",
> + pragma_or_attr, str_to_check, arg);
> + ret = false;
> + }
> + break;
> + }
> + default:
> + gcc_unreachable ();
> + }
> + }
> +
> + return ret;
> +}
> +#undef SKIP_WHITE
> +
> +/* Count how many times the character C appears in
> + NULL-terminated string STR. */
> +
> +static int
> +num_occurences_in_str (char c, char *str)
> +{
> + int res = 0;
> + while (*str != '\0')
> + {
> + if (*str == c)
> + res++;
> +
> + str++;
> + }
> +
> + return res;
> +}
int? unsigned surely?
> +
> +/* Parse the tree in ARGS that contains the target attribute information
> + and update the global target options space. PRAGMA_OR_ATTR is a string
> + to be used in error messages, specifying whether this is processing
> + a target attribute or a target pragma. */
> +
> +bool
> +aarch64_process_target_attr (tree args, const char* pragma_or_attr)
> +{
> + bool ret = true;
> + if (TREE_CODE (args) == TREE_LIST)
> + {
> + do
> + {
> + tree head = TREE_VALUE (args);
> + if (head)
> + {
> + bool ret2 = aarch64_process_target_attr (head, pragma_or_attr);
> + if (!ret2)
> + ret = false;
if (!aarch64_process_target_attr (head, pragma_or_attr))
ret = false;
> + }
> + args = TREE_CHAIN (args);
> + } while (args);
> +
> + return ret;
> + }
> + /* We expect to find a string to parse. */
> + else if (TREE_CODE (args) != STRING_CST)
> + gcc_unreachable ();
Just:
/* We expect to find a string to parse. */
gcc_assert (TREE_CODE (args) == STRING_CST)
> +
> + char *p = ASTRDUP (TREE_STRING_POINTER (args));
Another different way of duplicating a string in the same call chain...
> + char *str_to_check = p;
Is p used again, why the extra assignment?
> + int len = strlen (p);
strlen returns a size_t.
> +
> + if (len <= 0)
if (!len) size_t can't be negative.
> + {
> + error ("malformed target %s value", pragma_or_attr);
> + return false;
> + }
> +
> + /* Used to keep track of commas to catch situations where
> + invalid strings containing commas, but no attributes. */
Rephrase this please.
> + int num_commas = num_occurences_in_str (',', str_to_check);
> +
> + /* Handle multiple target attributes separated by ','. */
> + char *token = strtok (str_to_check, ",");
> +
> + int num_attrs = 0;
unsigned.
> + while (token)
> + {
> + num_attrs++;
> + bool tmp_ret = aarch64_process_one_target_attr (token, pragma_or_attr);
> +
> + if (!tmp_ret)
> + error ("target %s %qs is invalid", pragma_or_attr, token);
> +
> + ret &= tmp_ret;
if (!aarch64_process_one_target_attr (token, pragma_or_attr))
{
error ("target %s %qs is invalid", pragma_or_attr, token);
ret = false;
}
> +
> + token = strtok (NULL, ",");
> + }
> +
> + if (num_attrs != num_commas + 1)
> + {
> + error ("malformed target %s list %qs",
> + pragma_or_attr, TREE_STRING_POINTER (args));
> + ret = false;
> + }
> + return ret;
> +}
> +
> +/* Implement TARGET_OPTION_VALID_ATTRIBUTE_P. This is used to
> + process attribute((target("..."))). */
> +
> +static bool
> +aarch64_option_valid_attribute_p (tree fndecl, tree, tree args, int)
> +{
> + struct cl_target_option cur_target;
> + bool ret;
> + tree old_optimize;
> + tree new_target, new_optimize;
> + tree existing_target = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
> + tree func_optimize = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
> +
> + old_optimize = build_optimization_node (&global_options);
> + func_optimize = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
> +
> + /* If the function changed the optimization levels as well as setting
> target
Long line.
> + options, start with the optimizations specified. */
> + if (func_optimize && func_optimize != old_optimize)
> + cl_optimization_restore (&global_options,
> + TREE_OPTIMIZATION (func_optimize));
> +
> + /* Save the current target options to restore at the end. */
> + cl_target_option_save (&cur_target, &global_options);
> +
> + /* If fndecl already has some target attributes applied to it, unpack
> + them so that we add this attribute on top of them, rather than
> + overwriting them. */
> + if (existing_target)
> + {
> + struct cl_target_option *existing_options
> + = TREE_TARGET_OPTION (existing_target);
> +
> + if (existing_options)
> + cl_target_option_restore (&global_options, existing_options);
> + }
> + else
> + cl_target_option_restore (&global_options, TREE_TARGET_OPTION
> (target_option_current_node));
Long line.
Thanks,
James
