On Fri, Jul 24, 2015 at 11:43:32AM +0100, Kyrill Tkachov wrote:
>
> On 21/07/15 16:37, James Greenhalgh wrote:
> > 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.
>
> Thanks, here's the updated patch.
> I used alloca for memory allocation to keep consistent with the rest of
> aarch64.c,
> fixed the error message issues, spacing and control flow issues you pointed
> out.
>
> How's this?
> +static bool
> +aarch64_process_one_target_attr (char *arg_str, const char* pragma_or_attr)
> +{
> + bool ret;
> + bool invert = false;
> +
<<snip>>
> + ret = false;
I don't think this variable is exactly what you want. Surely as soon
as we see a failure case, we want to return false. Otherwise we could
see junk,junk,ok_stuff and return true.
So I would drop "ret" and rewrite this to bail out as soon as we see an
error. The downside is we won't catch multiple errors that way.
The other thing to do would be to invert the sense of your flag to something
like "failed" and only set it where we fail. It depends what behaviour
you're looking for.
> + 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 an attribute that expects one, fail to match. */
> + 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;
> +
> + if (attr_need_arg_p ^ (arg != NULL))
> + {
> + error ("target %s %qs does not accept an argument",
> + pragma_or_attr, str_to_check);
> + return false;
> + }
> +
> + /* 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);
> + return false;
> + }
> +
> + switch (p_attr->attr_type)
> + {
> + /* Has a custom handler registered.
> + For example, cpu=, arch=, tune=. */
> + case aarch64_attr_custom:
> + gcc_assert (p_attr->handler);
> + 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;
> +}
> +
> +/* 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;
Same comment again...
> + if (TREE_CODE (args) == TREE_LIST)
> + {
> + do
> + {
> + tree head = TREE_VALUE (args);
> + if (head)
> + {
> + 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. */
> + gcc_assert (TREE_CODE (args) == STRING_CST);
> +
> + size_t len = strlen (TREE_STRING_POINTER (args));
> + char *str_to_check = (char *) alloca (len + 1);
> + strcpy (str_to_check, TREE_STRING_POINTER (args));
> +
> + if (len == 0)
> + {
> + error ("malformed target %s value", pragma_or_attr);
> + return false;
> + }
> +
> + /* Used to catch empty spaces between commas i.e.
> + attribute ((target ("attr1,,attr2"))). */
> + unsigned int num_commas = num_occurences_in_str (',', str_to_check);
> +
> + /* Handle multiple target attributes separated by ','. */
> + char *token = strtok (str_to_check, ",");
> +
> + unsigned int num_attrs = 0;
> + while (token)
> + {
> + num_attrs++;
> + 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 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));
> +
> +
> + ret = aarch64_process_target_attr (args, "attribute");
> +
> + /* Set up any additional state. */
> + if (ret)
> + {
> + aarch64_override_options_internal (&global_options);
> + new_target = build_target_option_node (&global_options);
> + }
> + else
> + new_target = NULL;
> +
> + new_optimize = build_optimization_node (&global_options);
> +
> + if (!new_target)
> + ret = false;
> +
Misleading newline, and another use of a "ret" variable.
Thanks,
James
