[PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

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.

Ok for trunk?

Thanks,
Kyrill

2015-07-16  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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64-attrs-8.patch --]
[-- Type: text/x-patch; name=aarch64-attrs-8.patch, Size: 19388 bytes --]

commit 61d61f318cfa12a7cb05c10829de7b147e3238ce
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Fri May 8 12:06:24 2015 +0100

    [AArch64][8/N] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index b3fd9dc..726c625 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -60,7 +60,7 @@ static const struct default_options aarch_option_optimization_table[] =
    respective component of -mcpu.  This logic is implemented
    in config/aarch64/aarch64.c:aarch64_override_options.  */
 
-static bool
+bool
 aarch64_handle_option (struct gcc_options *opts,
 		       struct gcc_options *opts_set ATTRIBUTE_UNUSED,
 		       const struct cl_decoded_option *decoded,
@@ -68,6 +68,7 @@ aarch64_handle_option (struct gcc_options *opts,
 {
   size_t code = decoded->opt_index;
   const char *arg = decoded->arg;
+  int val = decoded->value;
 
   switch (code)
     {
@@ -83,6 +84,22 @@ aarch64_handle_option (struct gcc_options *opts,
       opts->x_aarch64_tune_string = arg;
       return true;
 
+    case OPT_mgeneral_regs_only:
+      opts->x_target_flags |= MASK_GENERAL_REGS_ONLY;
+      return true;
+
+    case OPT_mfix_cortex_a53_835769:
+      opts->x_aarch64_fix_a53_err835769 = val;
+      return true;
+
+    case OPT_mstrict_align:
+      opts->x_target_flags |= MASK_STRICT_ALIGN;
+      return true;
+
+    case OPT_momit_leaf_frame_pointer:
+      opts->x_flag_omit_frame_pointer = val;
+      return true;
+
     default:
       return true;
     }
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index fc1cec7..3a5482d 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -22,6 +22,8 @@
 #ifndef GCC_AARCH64_PROTOS_H
 #define GCC_AARCH64_PROTOS_H
 
+#include "input.h"
+
 /*
   SYMBOL_CONTEXT_ADR
   The symbol is used in a load-address operation.
@@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
 extern void aarch64_final_prescan_insn (rtx_insn *);
 extern bool
 aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
+bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
+			     const struct cl_decoded_option *, location_t);
 void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
 int aarch64_ccmp_mode_to_code (enum machine_mode mode);
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index ff87631..0a6ed70 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -57,6 +57,8 @@
 #include "tm_p.h"
 #include "recog.h"
 #include "langhooks.h"
+#include "opts.h"
+#include "diagnostic.h"
 #include "diagnostic-core.h"
 #include "internal-fn.h"
 #include "gimple-fold.h"
@@ -7981,6 +7983,509 @@ aarch64_set_current_function (tree fndecl)
     }
 }
 
+/* Enum describing the various ways we can handle attributes.
+   In many cases we can reuse the generic option handling machinery.  */
+
+enum aarch64_attr_opt_type
+{
+  aarch64_attr_mask,	/* Attribute should set a bit in target_flags.  */
+  aarch64_attr_bool,	/* Attribute sets or unsets a boolean variable.  */
+  aarch64_attr_enum,	/* Attribute sets an enum variable.  */
+  aarch64_attr_custom	/* Attribute requires a custom handling function.  */
+};
+
+/* All the information needed to handle a target attribute.
+   NAME is the name of the attribute.
+   ATTR_TYPE specifies the type of behaviour of the attribute as described
+   in the definition of enum aarch64_attr_opt_type.
+   ALLOW_NEG is true if the attribute supports a "no-" form.
+   HANDLER is the function that takes the attribute string and whether
+   it is a pragma or attribute and handles the option.  It is needed only
+   when the ATTR_TYPE is aarch64_attr_custom.
+   OPT_NUM is the enum specifying the option that the attribute modifies.
+   This is needed for attributes that mirror the behaviour of a command-line
+   option, that is it has ATTR_TYPE aarch64_attr_mask, aarch64_attr_bool or
+   aarch64_attr_enum.  */
+
+struct aarch64_attribute_info
+{
+  const char *name;
+  enum aarch64_attr_opt_type attr_type;
+  bool allow_neg;
+  bool (*handler) (const char *, const char *);
+  enum opt_code opt_num;
+};
+
+/* Handle the ARCH_STR argument to the arch= target attribute.
+   PRAGMA_OR_ATTR is used in potential error messages.  */
+
+static bool
+aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
+{
+  const struct processor *tmp_arch = NULL;
+  enum aarch64_parse_opt_result parse_res
+    = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
+
+  if (parse_res == AARCH64_PARSE_OK)
+    {
+      gcc_assert (tmp_arch);
+      selected_arch = tmp_arch;
+      explicit_arch = selected_arch->arch;
+      return true;
+    }
+
+  switch (parse_res)
+    {
+      case AARCH64_PARSE_MISSING_ARG:
+	error ("missing arch name in target %s arch=%qs", pragma_or_attr, str);
+	break;
+      case AARCH64_PARSE_INVALID_ARG:
+	error ("unknown value %qs for target %s arch=", str, pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_FEATURE:
+	error ("invalid feature modifier in target %s arch=%qs",
+	       pragma_or_attr, str);
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return false;
+}
+
+/* Handle the argument CPU_STR to the cpu= target attribute.
+   PRAGMA_OR_ATTR is used in potential error messages.  */
+
+static bool
+aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr)
+{
+  const struct processor *tmp_cpu = NULL;
+  enum aarch64_parse_opt_result parse_res
+    = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags);
+
+  if (parse_res == AARCH64_PARSE_OK)
+    {
+      gcc_assert (tmp_cpu);
+      selected_tune = tmp_cpu;
+      explicit_tune_core = selected_tune->ident;
+
+      selected_arch = &all_architectures[tmp_cpu->arch];
+      explicit_arch = selected_arch->arch;
+      return true;
+    }
+
+  switch (parse_res)
+    {
+      case AARCH64_PARSE_MISSING_ARG:
+	error ("missing cpu name in target %s cpu=%qs", pragma_or_attr, str);
+	break;
+      case AARCH64_PARSE_INVALID_ARG:
+	error ("unknown value %qs for target %s cpu", pragma_or_attr, str);
+	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);
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return false;
+}
+
+/* Parse an isa  extensions target attribute string specified in STR.
+   For example "+fp+nosimd".  Show any errors if needed and return true
+   if successful.  Update aarch64_isa_flags to reflect the ISA features
+   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.  */
+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)
+
+/* 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);
+  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.  */
+      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;
+      /* 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.  */
+      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;
+	}
+
+      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);
+	    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;
+}
+
+/* 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;
+	    }
+	  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 ();
+
+  char *p = ASTRDUP (TREE_STRING_POINTER (args));
+  char *str_to_check = p;
+  int len = strlen (p);
+
+  if (len <= 0)
+    {
+      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.  */
+  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;
+  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;
+
+      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;
+
+  else if (fndecl && ret)
+    {
+      DECL_FUNCTION_SPECIFIC_TARGET (fndecl) = new_target;
+
+      if (old_optimize != new_optimize)
+	DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl) = new_optimize;
+    }
+
+  cl_target_option_restore (&global_options, &cur_target);
+
+  if (old_optimize != new_optimize)
+    cl_optimization_restore (&global_options,
+			     TREE_OPTIMIZATION (old_optimize));
+  return ret;
+}
+
 /* Return true if SYMBOL_REF X binds locally.  */
 
 static bool
@@ -12455,6 +12960,9 @@ aarch64_unspec_may_trap_p (const_rtx x, unsigned flags)
 #undef TARGET_SET_CURRENT_FUNCTION
 #define TARGET_SET_CURRENT_FUNCTION aarch64_set_current_function
 
+#undef TARGET_OPTION_VALID_ATTRIBUTE_P
+#define TARGET_OPTION_VALID_ATTRIBUTE_P aarch64_option_valid_attribute_p
+
 #undef TARGET_PASS_BY_REFERENCE
 #define TARGET_PASS_BY_REFERENCE aarch64_pass_by_reference
 

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[James Greenhalgh]

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

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[Marcus Shawcroft]

On 21 July 2015 at 16:37, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> On Thu, Jul 16, 2015 at 04:20:59PM +0100, Kyrill Tkachov wrote:

>> +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.

Agreed, we should be consistent.  Each of the other instances of
alloca/xstrdup that I've seen in this patch series are actually
superflous, we simply copy the string, use it then bin it, better to
remove the dup completely.

In the choice between xstrdup() and alloca(), since alloca() is an
option, better to choose alloca() and remove the need for each of the
explicit free() calls in the exit paths. The alloca() route  makes it
less likely that we accidentally introduce a space leak in the future.

Cheers
/Marcus

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 18826 bytes --]


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?

Thanks,
Kyrill

2015-07-24  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.
> <...>
>
>> +      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
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64-attrs-8.patch --]
[-- Type: text/x-patch; name=aarch64-attrs-8.patch, Size: 19045 bytes --]

commit a75ecc3d124d2920ac56af66222ccaa2f14fa076
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Fri May 8 12:06:24 2015 +0100

    [AArch64][8/N] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index b3fd9dc..726c625 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -60,7 +60,7 @@ static const struct default_options aarch_option_optimization_table[] =
    respective component of -mcpu.  This logic is implemented
    in config/aarch64/aarch64.c:aarch64_override_options.  */
 
-static bool
+bool
 aarch64_handle_option (struct gcc_options *opts,
 		       struct gcc_options *opts_set ATTRIBUTE_UNUSED,
 		       const struct cl_decoded_option *decoded,
@@ -68,6 +68,7 @@ aarch64_handle_option (struct gcc_options *opts,
 {
   size_t code = decoded->opt_index;
   const char *arg = decoded->arg;
+  int val = decoded->value;
 
   switch (code)
     {
@@ -83,6 +84,22 @@ aarch64_handle_option (struct gcc_options *opts,
       opts->x_aarch64_tune_string = arg;
       return true;
 
+    case OPT_mgeneral_regs_only:
+      opts->x_target_flags |= MASK_GENERAL_REGS_ONLY;
+      return true;
+
+    case OPT_mfix_cortex_a53_835769:
+      opts->x_aarch64_fix_a53_err835769 = val;
+      return true;
+
+    case OPT_mstrict_align:
+      opts->x_target_flags |= MASK_STRICT_ALIGN;
+      return true;
+
+    case OPT_momit_leaf_frame_pointer:
+      opts->x_flag_omit_frame_pointer = val;
+      return true;
+
     default:
       return true;
     }
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index fc1cec7..3a5482d 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -22,6 +22,8 @@
 #ifndef GCC_AARCH64_PROTOS_H
 #define GCC_AARCH64_PROTOS_H
 
+#include "input.h"
+
 /*
   SYMBOL_CONTEXT_ADR
   The symbol is used in a load-address operation.
@@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
 extern void aarch64_final_prescan_insn (rtx_insn *);
 extern bool
 aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
+bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
+			     const struct cl_decoded_option *, location_t);
 void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
 int aarch64_ccmp_mode_to_code (enum machine_mode mode);
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 4e2ba5f..7314132 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -57,6 +57,8 @@
 #include "tm_p.h"
 #include "recog.h"
 #include "langhooks.h"
+#include "opts.h"
+#include "diagnostic.h"
 #include "diagnostic-core.h"
 #include "internal-fn.h"
 #include "gimple-fold.h"
@@ -7957,6 +7959,511 @@ aarch64_set_current_function (tree fndecl)
     }
 }
 
+/* Enum describing the various ways we can handle attributes.
+   In many cases we can reuse the generic option handling machinery.  */
+
+enum aarch64_attr_opt_type
+{
+  aarch64_attr_mask,	/* Attribute should set a bit in target_flags.  */
+  aarch64_attr_bool,	/* Attribute sets or unsets a boolean variable.  */
+  aarch64_attr_enum,	/* Attribute sets an enum variable.  */
+  aarch64_attr_custom	/* Attribute requires a custom handling function.  */
+};
+
+/* All the information needed to handle a target attribute.
+   NAME is the name of the attribute.
+   ATTR_TYPE specifies the type of behaviour of the attribute as described
+   in the definition of enum aarch64_attr_opt_type.
+   ALLOW_NEG is true if the attribute supports a "no-" form.
+   HANDLER is the function that takes the attribute string and whether
+   it is a pragma or attribute and handles the option.  It is needed only
+   when the ATTR_TYPE is aarch64_attr_custom.
+   OPT_NUM is the enum specifying the option that the attribute modifies.
+   This is needed for attributes that mirror the behaviour of a command-line
+   option, that is it has ATTR_TYPE aarch64_attr_mask, aarch64_attr_bool or
+   aarch64_attr_enum.  */
+
+struct aarch64_attribute_info
+{
+  const char *name;
+  enum aarch64_attr_opt_type attr_type;
+  bool allow_neg;
+  bool (*handler) (const char *, const char *);
+  enum opt_code opt_num;
+};
+
+/* Handle the ARCH_STR argument to the arch= target attribute.
+   PRAGMA_OR_ATTR is used in potential error messages.  */
+
+static bool
+aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
+{
+  const struct processor *tmp_arch = NULL;
+  enum aarch64_parse_opt_result parse_res
+    = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
+
+  if (parse_res == AARCH64_PARSE_OK)
+    {
+      gcc_assert (tmp_arch);
+      selected_arch = tmp_arch;
+      explicit_arch = selected_arch->arch;
+      return true;
+    }
+
+  switch (parse_res)
+    {
+      case AARCH64_PARSE_MISSING_ARG:
+	error ("missing architecture name in 'arch' target %s", pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_ARG:
+	error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_FEATURE:
+	error ("invalid feature modifier %qs for 'arch' target %s",
+	       str, pragma_or_attr);
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return false;
+}
+
+/* Handle the argument CPU_STR to the cpu= target attribute.
+   PRAGMA_OR_ATTR is used in potential error messages.  */
+
+static bool
+aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr)
+{
+  const struct processor *tmp_cpu = NULL;
+  enum aarch64_parse_opt_result parse_res
+    = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags);
+
+  if (parse_res == AARCH64_PARSE_OK)
+    {
+      gcc_assert (tmp_cpu);
+      selected_tune = tmp_cpu;
+      explicit_tune_core = selected_tune->ident;
+
+      selected_arch = &all_architectures[tmp_cpu->arch];
+      explicit_arch = selected_arch->arch;
+      return true;
+    }
+
+  switch (parse_res)
+    {
+      case AARCH64_PARSE_MISSING_ARG:
+	error ("missing cpu name in 'cpu' target %s", pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_ARG:
+	error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_FEATURE:
+	error ("invalid feature modifier %qs for 'cpu' target %s",
+	       str, pragma_or_attr);
+	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 'tune' target %s", str, pragma_or_attr);
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return false;
+}
+
+/* Parse an architecture extensions target attribute string specified in STR.
+   For example "+fp+nosimd".  Show any errors if needed.  Return TRUE
+   if successful.  Update aarch64_isa_flags to reflect the ISA features
+   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.  */
+
+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____ }
+};
+
+/* 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;
+
+  size_t len = strlen (arg_str);
+
+  if (len == 0)
+    {
+      error ("malformed target %s", pragma_or_attr);
+      return false;
+    }
+
+  char *str_to_check = (char *) alloca (len + 1);
+  strcpy (str_to_check, arg_str);
+
+  /* Skip leading whitespace.  */
+  while (*str_to_check == ' ' || *str_to_check == '\t')
+    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 == '+')
+    {
+      ret = aarch64_handle_attr_isa_flags (str_to_check, pragma_or_attr);
+      return ret;
+    }
+
+  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 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;
+}
+
+/* Count how many times the character C appears in
+   NULL-terminated string STR.  */
+
+static unsigned int
+num_occurences_in_str (char c, char *str)
+{
+  unsigned int res = 0;
+  while (*str != '\0')
+    {
+      if (*str == c)
+	res++;
+
+      str++;
+    }
+
+  return res;
+}
+
+/* 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)
+	    {
+	      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;
+
+  else if (fndecl && ret)
+    {
+      DECL_FUNCTION_SPECIFIC_TARGET (fndecl) = new_target;
+
+      if (old_optimize != new_optimize)
+	DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl) = new_optimize;
+    }
+
+  cl_target_option_restore (&global_options, &cur_target);
+
+  if (old_optimize != new_optimize)
+    cl_optimization_restore (&global_options,
+			     TREE_OPTIMIZATION (old_optimize));
+  return ret;
+}
+
 /* Return true if SYMBOL_REF X binds locally.  */
 
 static bool
@@ -12431,6 +12938,9 @@ aarch64_unspec_may_trap_p (const_rtx x, unsigned flags)
 #undef TARGET_OPTION_PRINT
 #define TARGET_OPTION_PRINT aarch64_option_print
 
+#undef TARGET_OPTION_VALID_ATTRIBUTE_P
+#define TARGET_OPTION_VALID_ATTRIBUTE_P aarch64_option_valid_attribute_p
+
 #undef TARGET_SET_CURRENT_FUNCTION
 #define TARGET_SET_CURRENT_FUNCTION aarch64_set_current_function
 

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[James Greenhalgh]

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

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 11327 bytes --]


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
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64-attrs-8.patch --]
[-- Type: text/x-patch; name=aarch64-attrs-8.patch, Size: 18830 bytes --]

commit 48f8c09d7841c2591714c7ca90fb2894fc9186b1
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Fri May 8 12:06:24 2015 +0100

    [AArch64][8/N] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index b3fd9dc..726c625 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -60,7 +60,7 @@ static const struct default_options aarch_option_optimization_table[] =
    respective component of -mcpu.  This logic is implemented
    in config/aarch64/aarch64.c:aarch64_override_options.  */
 
-static bool
+bool
 aarch64_handle_option (struct gcc_options *opts,
 		       struct gcc_options *opts_set ATTRIBUTE_UNUSED,
 		       const struct cl_decoded_option *decoded,
@@ -68,6 +68,7 @@ aarch64_handle_option (struct gcc_options *opts,
 {
   size_t code = decoded->opt_index;
   const char *arg = decoded->arg;
+  int val = decoded->value;
 
   switch (code)
     {
@@ -83,6 +84,22 @@ aarch64_handle_option (struct gcc_options *opts,
       opts->x_aarch64_tune_string = arg;
       return true;
 
+    case OPT_mgeneral_regs_only:
+      opts->x_target_flags |= MASK_GENERAL_REGS_ONLY;
+      return true;
+
+    case OPT_mfix_cortex_a53_835769:
+      opts->x_aarch64_fix_a53_err835769 = val;
+      return true;
+
+    case OPT_mstrict_align:
+      opts->x_target_flags |= MASK_STRICT_ALIGN;
+      return true;
+
+    case OPT_momit_leaf_frame_pointer:
+      opts->x_flag_omit_frame_pointer = val;
+      return true;
+
     default:
       return true;
     }
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index fc1cec7..3a5482d 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -22,6 +22,8 @@
 #ifndef GCC_AARCH64_PROTOS_H
 #define GCC_AARCH64_PROTOS_H
 
+#include "input.h"
+
 /*
   SYMBOL_CONTEXT_ADR
   The symbol is used in a load-address operation.
@@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
 extern void aarch64_final_prescan_insn (rtx_insn *);
 extern bool
 aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
+bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
+			     const struct cl_decoded_option *, location_t);
 void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
 int aarch64_ccmp_mode_to_code (enum machine_mode mode);
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index d0d62e7..7a369fd 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -57,6 +57,8 @@
 #include "tm_p.h"
 #include "recog.h"
 #include "langhooks.h"
+#include "opts.h"
+#include "diagnostic.h"
 #include "diagnostic-core.h"
 #include "internal-fn.h"
 #include "gimple-fold.h"
@@ -7964,6 +7966,499 @@ aarch64_set_current_function (tree fndecl)
     }
 }
 
+/* Enum describing the various ways we can handle attributes.
+   In many cases we can reuse the generic option handling machinery.  */
+
+enum aarch64_attr_opt_type
+{
+  aarch64_attr_mask,	/* Attribute should set a bit in target_flags.  */
+  aarch64_attr_bool,	/* Attribute sets or unsets a boolean variable.  */
+  aarch64_attr_enum,	/* Attribute sets an enum variable.  */
+  aarch64_attr_custom	/* Attribute requires a custom handling function.  */
+};
+
+/* All the information needed to handle a target attribute.
+   NAME is the name of the attribute.
+   ATTR_TYPE specifies the type of behaviour of the attribute as described
+   in the definition of enum aarch64_attr_opt_type.
+   ALLOW_NEG is true if the attribute supports a "no-" form.
+   HANDLER is the function that takes the attribute string and whether
+   it is a pragma or attribute and handles the option.  It is needed only
+   when the ATTR_TYPE is aarch64_attr_custom.
+   OPT_NUM is the enum specifying the option that the attribute modifies.
+   This is needed for attributes that mirror the behaviour of a command-line
+   option, that is it has ATTR_TYPE aarch64_attr_mask, aarch64_attr_bool or
+   aarch64_attr_enum.  */
+
+struct aarch64_attribute_info
+{
+  const char *name;
+  enum aarch64_attr_opt_type attr_type;
+  bool allow_neg;
+  bool (*handler) (const char *, const char *);
+  enum opt_code opt_num;
+};
+
+/* Handle the ARCH_STR argument to the arch= target attribute.
+   PRAGMA_OR_ATTR is used in potential error messages.  */
+
+static bool
+aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
+{
+  const struct processor *tmp_arch = NULL;
+  enum aarch64_parse_opt_result parse_res
+    = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
+
+  if (parse_res == AARCH64_PARSE_OK)
+    {
+      gcc_assert (tmp_arch);
+      selected_arch = tmp_arch;
+      explicit_arch = selected_arch->arch;
+      return true;
+    }
+
+  switch (parse_res)
+    {
+      case AARCH64_PARSE_MISSING_ARG:
+	error ("missing architecture name in 'arch' target %s", pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_ARG:
+	error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_FEATURE:
+	error ("invalid feature modifier %qs for 'arch' target %s",
+	       str, pragma_or_attr);
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return false;
+}
+
+/* Handle the argument CPU_STR to the cpu= target attribute.
+   PRAGMA_OR_ATTR is used in potential error messages.  */
+
+static bool
+aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr)
+{
+  const struct processor *tmp_cpu = NULL;
+  enum aarch64_parse_opt_result parse_res
+    = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags);
+
+  if (parse_res == AARCH64_PARSE_OK)
+    {
+      gcc_assert (tmp_cpu);
+      selected_tune = tmp_cpu;
+      explicit_tune_core = selected_tune->ident;
+
+      selected_arch = &all_architectures[tmp_cpu->arch];
+      explicit_arch = selected_arch->arch;
+      return true;
+    }
+
+  switch (parse_res)
+    {
+      case AARCH64_PARSE_MISSING_ARG:
+	error ("missing cpu name in 'cpu' target %s", pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_ARG:
+	error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr);
+	break;
+      case AARCH64_PARSE_INVALID_FEATURE:
+	error ("invalid feature modifier %qs for 'cpu' target %s",
+	       str, pragma_or_attr);
+	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 'tune' target %s", str, pragma_or_attr);
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return false;
+}
+
+/* Parse an architecture extensions target attribute string specified in STR.
+   For example "+fp+nosimd".  Show any errors if needed.  Return TRUE
+   if successful.  Update aarch64_isa_flags to reflect the ISA features
+   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.  */
+
+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____ }
+};
+
+/* 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 invert = false;
+
+  size_t len = strlen (arg_str);
+
+  if (len == 0)
+    {
+      error ("malformed target %s", pragma_or_attr);
+      return false;
+    }
+
+  char *str_to_check = (char *) alloca (len + 1);
+  strcpy (str_to_check, arg_str);
+
+  /* Skip leading whitespace.  */
+  while (*str_to_check == ' ' || *str_to_check == '\t')
+    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;
+  for (p_attr = aarch64_attributes; 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);
+	    if (!p_attr->handler (arg, pragma_or_attr))
+	      return false;
+	    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);
+	      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);
+	      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);
+		}
+	      else
+		{
+		  error ("target %s %s=%s is not valid",
+			 pragma_or_attr, str_to_check, arg);
+		}
+	      break;
+	    }
+	  default:
+	    gcc_unreachable ();
+	}
+    }
+
+  return true;
+}
+
+/* Count how many times the character C appears in
+   NULL-terminated string STR.  */
+
+static unsigned int
+num_occurences_in_str (char c, char *str)
+{
+  unsigned int res = 0;
+  while (*str != '\0')
+    {
+      if (*str == c)
+	res++;
+
+      str++;
+    }
+
+  return res;
+}
+
+/* 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)
+{
+  if (TREE_CODE (args) == TREE_LIST)
+    {
+      do
+	{
+	  tree head = TREE_VALUE (args);
+	  if (head)
+	    {
+	      if (!aarch64_process_target_attr (head, pragma_or_attr))
+		return false;
+	    }
+	  args = TREE_CHAIN (args);
+	} while (args);
+
+      return true;
+    }
+  /* 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);
+	  return false;
+	}
+
+      token = strtok (NULL, ",");
+    }
+
+  if (num_attrs != num_commas + 1)
+    {
+      error ("malformed target %s list %qs",
+	      pragma_or_attr, TREE_STRING_POINTER (args));
+      return false;
+    }
+
+  return true;
+}
+
+/* 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 (fndecl && ret)
+    {
+      DECL_FUNCTION_SPECIFIC_TARGET (fndecl) = new_target;
+
+      if (old_optimize != new_optimize)
+	DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl) = new_optimize;
+    }
+
+  cl_target_option_restore (&global_options, &cur_target);
+
+  if (old_optimize != new_optimize)
+    cl_optimization_restore (&global_options,
+			     TREE_OPTIMIZATION (old_optimize));
+  return ret;
+}
+
 /* Return true if SYMBOL_REF X binds locally.  */
 
 static bool
@@ -12478,6 +12973,9 @@ aarch64_promoted_type (const_tree t)
 #undef TARGET_OPTION_PRINT
 #define TARGET_OPTION_PRINT aarch64_option_print
 
+#undef TARGET_OPTION_VALID_ATTRIBUTE_P
+#define TARGET_OPTION_VALID_ATTRIBUTE_P aarch64_option_valid_attribute_p
+
 #undef TARGET_SET_CURRENT_FUNCTION
 #define TARGET_SET_CURRENT_FUNCTION aarch64_set_current_function
 

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[James Greenhalgh]

On Mon, Aug 03, 2015 at 04:20:13PM +0100, Kyrill Tkachov wrote:
> 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.

This is OK with the changes below fixed, or commented on as justification.

> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index fc1cec7..3a5482d 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
>  extern void aarch64_final_prescan_insn (rtx_insn *);
>  extern bool
>  aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
> +bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
> +			     const struct cl_decoded_option *, location_t);

Please try to keep this file in alphabetical order, first by return type,
then by function name.

>  void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
>  int aarch64_ccmp_mode_to_code (enum machine_mode mode);
>  
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index d0d62e7..7a369fd 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c

> +static bool
> +aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
> +{
> +  const struct processor *tmp_arch = NULL;
> +  enum aarch64_parse_opt_result parse_res
> +    = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
> +
> +  if (parse_res == AARCH64_PARSE_OK)
> +    {
> +      gcc_assert (tmp_arch);
> +      selected_arch = tmp_arch;
> +      explicit_arch = selected_arch->arch;
> +      return true;
> +    }

Why not pull this in to the switch case below?

> +
> +  switch (parse_res)
> +    {
> +      case AARCH64_PARSE_MISSING_ARG:
> +	error ("missing architecture name in 'arch' target %s", pragma_or_attr);
> +	break;
> +      case AARCH64_PARSE_INVALID_ARG:
> +	error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr);
> +	break;
> +      case AARCH64_PARSE_INVALID_FEATURE:
> +	error ("invalid feature modifier %qs for 'arch' target %s",
> +	       str, pragma_or_attr);
> +	break;
> +      default:
> +	gcc_unreachable ();
> +    }
> +
> +  return false;
> +}
> +
> +/* Handle the argument CPU_STR to the cpu= target attribute.
> +   PRAGMA_OR_ATTR is used in potential error messages.  */
> +
> +static bool
> +aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr)
> +{
> +  const struct processor *tmp_cpu = NULL;
> +  enum aarch64_parse_opt_result parse_res
> +    = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags);
> +
> +  if (parse_res == AARCH64_PARSE_OK)
> +    {
> +      gcc_assert (tmp_cpu);
> +      selected_tune = tmp_cpu;
> +      explicit_tune_core = selected_tune->ident;
> +
> +      selected_arch = &all_architectures[tmp_cpu->arch];
> +      explicit_arch = selected_arch->arch;
> +      return true;
> +    }

Likewise here.

> +
> +  switch (parse_res)
> +    {
> +      case AARCH64_PARSE_MISSING_ARG:
> +	error ("missing cpu name in 'cpu' target %s", pragma_or_attr);
> +	break;
> +      case AARCH64_PARSE_INVALID_ARG:
> +	error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr);
> +	break;
> +      case AARCH64_PARSE_INVALID_FEATURE:
> +	error ("invalid feature modifier %qs for 'cpu' target %s",
> +	       str, pragma_or_attr);
> +	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;
> +    }
> +

And likewise here.

> +  switch (parse_res)
> +    {
> +      case AARCH64_PARSE_INVALID_ARG:
> +	error ("unknown value %qs for 'tune' target %s", str, pragma_or_attr);
> +	break;
> +      default:
> +	gcc_unreachable ();
> +    }
> +
> +  return false;
> +}
> +
> +/* Parse an architecture extensions target attribute string specified in STR.
> +   For example "+fp+nosimd".  Show any errors if needed.  Return TRUE
> +   if successful.  Update aarch64_isa_flags to reflect the ISA features
> +   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;
> +    }
> +

And again here.

> +  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;
> +}
> +

Thanks,
James

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[Kyrill Tkachov]

On 04/08/15 09:53, James Greenhalgh wrote:
> On Mon, Aug 03, 2015 at 04:20:13PM +0100, Kyrill Tkachov wrote:
>> 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.
> This is OK with the changes below fixed, or commented on as justification.
>
>> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
>> index fc1cec7..3a5482d 100644
>> --- a/gcc/config/aarch64/aarch64-protos.h
>> +++ b/gcc/config/aarch64/aarch64-protos.h
>> @@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
>>   extern void aarch64_final_prescan_insn (rtx_insn *);
>>   extern bool
>>   aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
>> +bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
>> +			     const struct cl_decoded_option *, location_t);
> Please try to keep this file in alphabetical order, first by return type,
> then by function name.

Ok, will do.

>
>>   void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
>>   int aarch64_ccmp_mode_to_code (enum machine_mode mode);
>>   
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index d0d62e7..7a369fd 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> +static bool
>> +aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
>> +{
>> +  const struct processor *tmp_arch = NULL;
>> +  enum aarch64_parse_opt_result parse_res
>> +    = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
>> +
>> +  if (parse_res == AARCH64_PARSE_OK)
>> +    {
>> +      gcc_assert (tmp_arch);
>> +      selected_arch = tmp_arch;
>> +      explicit_arch = selected_arch->arch;
>> +      return true;
>> +    }
> Why not pull this in to the switch case below?

I chose to keep the success case separate from error handling and reporting as it made it
easier to find it (and it is the more interesting case in these functions). I can add a comment
to that effect there if you'd like.

Thanks,
Kyrill

>
>> +
>> +  switch (parse_res)
>> +    {
>> +      case AARCH64_PARSE_MISSING_ARG:
>> +	error ("missing architecture name in 'arch' target %s", pragma_or_attr);
>> +	break;
>> +      case AARCH64_PARSE_INVALID_ARG:
>> +	error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr);
>> +	break;
>> +      case AARCH64_PARSE_INVALID_FEATURE:
>> +	error ("invalid feature modifier %qs for 'arch' target %s",
>> +	       str, pragma_or_attr);
>> +	break;
>> +      default:
>> +	gcc_unreachable ();
>> +    }
>> +
>> +  return false;
>> +}
>> +
>> +/* Handle the argument CPU_STR to the cpu= target attribute.
>> +   PRAGMA_OR_ATTR is used in potential error messages.  */
>> +
>> +static bool
>> +aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr)
>> +{
>> +  const struct processor *tmp_cpu = NULL;
>> +  enum aarch64_parse_opt_result parse_res
>> +    = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags);
>> +
>> +  if (parse_res == AARCH64_PARSE_OK)
>> +    {
>> +      gcc_assert (tmp_cpu);
>> +      selected_tune = tmp_cpu;
>> +      explicit_tune_core = selected_tune->ident;
>> +
>> +      selected_arch = &all_architectures[tmp_cpu->arch];
>> +      explicit_arch = selected_arch->arch;
>> +      return true;
>> +    }
> Likewise here.
>
>> +
>> +  switch (parse_res)
>> +    {
>> +      case AARCH64_PARSE_MISSING_ARG:
>> +	error ("missing cpu name in 'cpu' target %s", pragma_or_attr);
>> +	break;
>> +      case AARCH64_PARSE_INVALID_ARG:
>> +	error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr);
>> +	break;
>> +      case AARCH64_PARSE_INVALID_FEATURE:
>> +	error ("invalid feature modifier %qs for 'cpu' target %s",
>> +	       str, pragma_or_attr);
>> +	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;
>> +    }
>> +
> And likewise here.
>
>> +  switch (parse_res)
>> +    {
>> +      case AARCH64_PARSE_INVALID_ARG:
>> +	error ("unknown value %qs for 'tune' target %s", str, pragma_or_attr);
>> +	break;
>> +      default:
>> +	gcc_unreachable ();
>> +    }
>> +
>> +  return false;
>> +}
>> +
>> +/* Parse an architecture extensions target attribute string specified in STR.
>> +   For example "+fp+nosimd".  Show any errors if needed.  Return TRUE
>> +   if successful.  Update aarch64_isa_flags to reflect the ISA features
>> +   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;
>> +    }
>> +
> And again here.
>
>> +  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;
>> +}
>> +
> Thanks,
> James
>

Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P

[James Greenhalgh]

On Tue, Aug 04, 2015 at 09:58:37AM +0100, Kyrill Tkachov wrote:
> 
> On 04/08/15 09:53, James Greenhalgh wrote:
> > On Mon, Aug 03, 2015 at 04:20:13PM +0100, Kyrill Tkachov wrote:
> >> 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.
> > This is OK with the changes below fixed, or commented on as justification.
> >
> >> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> >> index fc1cec7..3a5482d 100644
> >> --- a/gcc/config/aarch64/aarch64-protos.h
> >> +++ b/gcc/config/aarch64/aarch64-protos.h
> >> @@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
> >>   extern void aarch64_final_prescan_insn (rtx_insn *);
> >>   extern bool
> >>   aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
> >> +bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
> >> +			     const struct cl_decoded_option *, location_t);
> > Please try to keep this file in alphabetical order, first by return type,
> > then by function name.
> 
> Ok, will do.
> 
> >
> >>   void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
> >>   int aarch64_ccmp_mode_to_code (enum machine_mode mode);
> >>   
> >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> index d0d62e7..7a369fd 100644
> >> --- a/gcc/config/aarch64/aarch64.c
> >> +++ b/gcc/config/aarch64/aarch64.c
> >> +static bool
> >> +aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
> >> +{
> >> +  const struct processor *tmp_arch = NULL;
> >> +  enum aarch64_parse_opt_result parse_res
> >> +    = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
> >> +
> >> +  if (parse_res == AARCH64_PARSE_OK)
> >> +    {
> >> +      gcc_assert (tmp_arch);
> >> +      selected_arch = tmp_arch;
> >> +      explicit_arch = selected_arch->arch;
> >> +      return true;
> >> +    }
> > Why not pull this in to the switch case below?
> 
> I chose to keep the success case separate from error handling and reporting as it made it
> easier to find it (and it is the more interesting case in these functions). I can add a comment
> to that effect there if you'd like.

I thought that might be it. It looks unusual to me, but I don't have
strong feelings against it, so I'm happy for you to leave it as is if
that is your preference.

Thanks,
James