From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 124143 invoked by alias); 21 Jul 2015 15:37:40 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 124122 invoked by uid 89); 21 Jul 2015 15:37:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 21 Jul 2015 15:37:37 +0000 Received: from arm.com (e106375-lin.cambridge.arm.com [10.2.207.23]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id t6LFbXJa010826; Tue, 21 Jul 2015 16:37:33 +0100 Date: Tue, 21 Jul 2015 16:07:00 -0000 From: James Greenhalgh To: Kyrill Tkachov Cc: GCC Patches , Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P Message-ID: <20150721153733.GA13935@arm.com> References: <55A7CBDB.9020908@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55A7CBDB.9020908@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg01768.txt.bz2 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= 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 for target attribute arch=" How about: "unknown value 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 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