[PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P
Kyrill Tkachov
kyrylo.tkachov@arm.com
Mon Aug 3 15:20:00 GMT 2015
On 03/08/15 11:52, James Greenhalgh wrote:
> 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.
Ok, I've removed usages of 'ret' in favor of returning when appropriate.
In this last one I left the ret (but cleaned up the control flow a bit)
because if the processing fails we need to clean up a bit of state before
returning.
Thanks,
Kyrill
2015-08-03 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
* common/config/aarch64/aarch64-common.c (aarch64_handle_option):
Remove static. Handle OPT_mgeneral_regs_only,
OPT_mfix_cortex_a53_835769, OPT_mstrict_align,
OPT_momit_leaf_frame_pointer.
* config/aarch64/aarch64.c: Include opts.h and diagnostic.h
(aarch64_attr_opt_type): New enum.
(aarch64_attribute_info): New struct.
(aarch64_handle_attr_arch): New function.
(aarch64_handle_attr_cpu): Likewise.
(aarch64_handle_attr_tune): Likewise.
(aarch64_handle_attr_isa_flags): Likewise.
(aarch64_attributes): New table.
(aarch64_process_one_target_attr): New function.
(num_occurences_in_str): Likewise.
(aarch64_process_target_attr): Likewise.
(aarch64_option_valid_attribute_p): Likewise.
(TARGET_OPTION_VALID_ATTRIBUTE_P): Define.
* config/aarch64/aarch64-protos.h: Include input.h
(aarch64_handle_option): Declare prototype.
>
> Thanks,
> James
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aarch64-attrs-8.patch
Type: text/x-patch
Size: 18235 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150803/292771ac/attachment.bin>
More information about the Gcc-patches
mailing list