public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [AArch64] Break -mcpu tie between the compiler and assembler
@ 2015-08-19 15:43 James Greenhalgh
  2015-08-19 15:48 ` Andrew Pinski
  2015-08-19 15:50 ` Andrew Pinski
  0 siblings, 2 replies; 5+ messages in thread
From: James Greenhalgh @ 2015-08-19 15:43 UTC (permalink / raw)
  To: gcc-patches; +Cc: marcus.shawcroft, richard.earnshaw

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


Hi,

This patch has been sitting in my tree for a while - it comes in handy
when trying out bootstrap or test with -mcpu values like -mcpu=cortex-a72
with a system assmebler which trails trunk binutils.

Essentially, we rewrite -mcpu=foo to a -march flag providing the same
architecture revision and set of optional architecture features. There
is no reason we should ever need the assembler to see a CPU name, it
should only be interested in the architecture variant.

While we're there I've long found this function too fragile and hard
to grok in C. So I've rewritten it in C++ to use std::string rather than
raw C strings. Making this work with extension strings requires a slight
refactor to the existing extension printing code to pull it across to
somewhere common.

Note that this also stops us from having to pick through a big.LITTLE
target to find and separate the core names - we can just look up the
architecture of the whole target and use that.

The new function does leak the allocation of a C string to hold the
result, but looking at gcc.c:getenv_spec_function and
gcc.c:replace_extension_spec_func this is the usual thing to do.

This has been through an aarch64-none-linux-gnu bootstrap and test run,
configured with --with-cpu=cortex-a72 , which my system assembler does
not understand.

Ok?

Thanks,
James

---
2015-08-19  James Greenhalgh  <james.greenhalgh@arm.com>

	* common/config/aarch64/aarch64-common.c
	(AARCH64_CPU_NAME_LENGTH): Delete.
	(aarch64_option_extension): New.
	(all_extensions): Likewise.
	(processor_name_to_arch): Likewise.
	(arch_to_arch_name): Likewise.
	(all_cores): New.
	(all_architectures): Likewise.
	(aarch64_get_extension_string_for_isa_flags): Likewise.
	(aarch64_rewrite_selected_cpu): Change to rewrite CPU names to
	architecture names.
	* config/aarch64/aarch64-protos.h
	(aarch64_get_extension_string_for_isa_flags): New.
	* config/aarch64/aarch64.c (aarch64_print_extension): Delete.
	(aarch64_option_print): Get the string to print from
	aarch64_get_extension_string_for_isa_flags.
	(aarch64_declare_function_name): Likewise.
	* config/aarch64/aarch64.h (BIG_LITTLE_SPEC): Rename to...
	(MCPU_TO_MARCH_SPEC): This.
	(ASM_CPU_SPEC): Use it.
	(BIG_LITTLE_SPEC_FUNCTIONS): Rename to...
	(MCPU_TO_MARCH_SPEC_FUNCTIONS): ...This.
	(EXTRA_SPEC_FUNCTIONS): Use it.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-AArch64-Break-mcpu-tie-between-the-compiler-and-asse.patch --]
[-- Type: text/x-patch;  name=0001-AArch64-Break-mcpu-tie-between-the-compiler-and-asse.patch, Size: 9994 bytes --]

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 726c625..476401f 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -27,7 +27,7 @@
 #include "common/common-target-def.h"
 #include "opts.h"
 #include "flags.h"
-#include "errors.h"
+#include "diagnostic.h"
 
 #ifdef  TARGET_BIG_ENDIAN_DEFAULT
 #undef  TARGET_DEFAULT_TARGET_FLAGS
@@ -107,36 +107,137 @@ aarch64_handle_option (struct gcc_options *opts,
 
 struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
 
-#define AARCH64_CPU_NAME_LENGTH 128
+/* An ISA extension in the co-processor and main instruction set space.  */
+struct aarch64_option_extension
+{
+  const char *const name;
+  const unsigned long flags_on;
+  const unsigned long flags_off;
+};
+
+/* ISA extensions in AArch64.  */
+static const struct aarch64_option_extension all_extensions[] =
+{
+#define AARCH64_OPT_EXTENSION(NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
+  {NAME, FLAGS_ON, FLAGS_OFF},
+#include "config/aarch64/aarch64-option-extensions.def"
+#undef AARCH64_OPT_EXTENSION
+  {NULL, 0, 0}
+};
+
+struct processor_name_to_arch
+{
+  const std::string processor_name;
+  const enum aarch64_arch arch;
+  const unsigned long flags;
+};
+
+struct arch_to_arch_name
+{
+  const enum aarch64_arch arch;
+  const std::string arch_name;
+};
+
+/* Map processor names to the architecture revision they implement and
+   the default set of architectural feature flags they support.  */
+static const struct processor_name_to_arch all_cores[] =
+{
+#define AARCH64_CORE(NAME, X, IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART) \
+  {NAME, AARCH64_ARCH_##ARCH_IDENT, FLAGS},
+#include "config/aarch64/aarch64-cores.def"
+#undef AARCH64_CORE
+  {"generic", AARCH64_ARCH_8A, AARCH64_FL_FOR_ARCH8},
+  {"", aarch64_no_arch, 0}
+};
+
+/* Map architecture revisions to their string representation.  */
+static const struct arch_to_arch_name all_architectures[] =
+{
+#define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH, FLAGS) \
+  {AARCH64_ARCH_##ARCH_IDENT, NAME},
+#include "config/aarch64/aarch64-arches.def"
+#undef AARCH64_ARCH
+  {aarch64_no_arch, ""}
+};
+
+/* Return a string representation of ISA_FLAGS.  */
+
+std::string
+aarch64_get_extension_string_for_isa_flags (unsigned long isa_flags)
+{
+  const struct aarch64_option_extension *opt = NULL;
+  std::string outstr = "";
+
+  for (opt = all_extensions; opt->name != NULL; opt++)
+    if ((isa_flags & opt->flags_on) == opt->flags_on)
+      {
+	outstr += "+";
+	outstr += opt->name;
+      }
+  return outstr;
+}
 
-/* Truncate NAME at the first '.' character seen up to the first '+'
-   or return NAME unmodified.  */
+/* Attempt to rewrite NAME, which has been passed on the command line
+   as a -mcpu option to an equivalent -march value.  If we can do so,
+   return the new string, otherwise return an error.  */
 
 const char *
 aarch64_rewrite_selected_cpu (const char *name)
 {
-  static char output_buf[AARCH64_CPU_NAME_LENGTH + 1] = {0};
-  const char *bL_sep;
-  const char *feats;
-  size_t pref_size;
-  size_t feat_size;
-
-  bL_sep = strchr (name, '.');
-  if (!bL_sep)
-    return name;
+  std::string original_string (name);
+  std::string extensions;
+  std::string processor;
+  size_t extension_pos = original_string.find_first_of ('+');
 
-  feats = strchr (name, '+');
-  feat_size = feats ? strnlen (feats, AARCH64_CPU_NAME_LENGTH) : 0;
-  pref_size = bL_sep - name;
+  /* Strip and save the extension string.  */
+  if (extension_pos != std::string::npos)
+    {
+      processor = original_string.substr (0, extension_pos);
+      extensions = original_string.substr (extension_pos,
+					std::string::npos);
+    }
+  else
+    {
+      /* No extensions.  */
+      processor = original_string;
+    }
 
-  if ((feat_size + pref_size) > AARCH64_CPU_NAME_LENGTH)
-    internal_error ("-mcpu string too large");
+  const struct processor_name_to_arch* p_to_a;
+  for (p_to_a = all_cores;
+       p_to_a->arch != aarch64_no_arch;
+       p_to_a++)
+    {
+      if (p_to_a->processor_name == processor)
+	break;
+    }
 
-  strncpy (output_buf, name, pref_size);
-  if (feats)
-    strncpy (output_buf + pref_size, feats, feat_size);
+  const struct arch_to_arch_name* a_to_an;
+  for (a_to_an = all_architectures;
+       a_to_an->arch != aarch64_no_arch;
+       a_to_an++)
+    {
+      if (a_to_an->arch == p_to_a->arch)
+	break;
+    }
 
-  return output_buf;
+  /* We couldn't find that proceesor name, or the processor name we
+     found does not map to an architecture we understand.  */
+  if (p_to_a->arch == aarch64_no_arch
+      || a_to_an->arch == aarch64_no_arch)
+    fatal_error (input_location, "unknown value %qs for -mcpu", name);
+
+  std::string outstr = a_to_an->arch_name
+	+ aarch64_get_extension_string_for_isa_flags (p_to_a->flags)
+	+ extensions;
+
+  /* We are going to memory leak here, nobody elsewhere
+     in the callchain is going to clean up after us.  The alternative is
+     to allocate a static buffer, and assert that it is big enough for our
+     modified string, which seems much worse!  */
+  char *output = (char*) xmalloc (sizeof (*output)
+				  * (outstr.length () + 1));
+  strcpy (output, outstr.c_str ());
+  return output;
 }
 
 /* Called by the driver to rewrite a name passed to the -mcpu
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 0b09d49..06002d8 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -310,6 +310,7 @@ rtx aarch64_simd_gen_const_vector_dup (machine_mode, int);
 bool aarch64_simd_mem_operand_p (rtx);
 rtx aarch64_simd_vect_par_cnst_half (machine_mode, bool);
 rtx aarch64_tls_get_addr (void);
+std::string aarch64_get_extension_string_for_isa_flags (unsigned long);
 tree aarch64_fold_builtin (tree, int, tree *, bool);
 unsigned aarch64_dbx_register_number (unsigned);
 unsigned aarch64_trampoline_size (void);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index aa268ae..db316a0 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7904,20 +7904,6 @@ initialize_aarch64_code_model (struct gcc_options *opts)
      aarch64_cmodel = opts->x_aarch64_cmodel_var;
 }
 
-/* Print to F the architecture features specified by ISA_FLAGS.  */
-
-static void
-aarch64_print_extension (FILE *f, unsigned long isa_flags)
-{
-  const struct aarch64_option_extension *opt = NULL;
-
-  for (opt = all_extensions; opt->name != NULL; opt++)
-    if ((isa_flags & opt->flags_on) == opt->flags_on)
-      asm_fprintf (f, "+%s", opt->name);
-
-  asm_fprintf (f, "\n");
-}
-
 /* Implement TARGET_OPTION_SAVE.  */
 
 static void
@@ -7950,10 +7936,12 @@ aarch64_option_print (FILE *file, int indent, struct cl_target_option *ptr)
     = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
   unsigned long isa_flags = ptr->x_aarch64_isa_flags;
   const struct processor *arch = aarch64_get_arch (ptr->x_explicit_arch);
+  std::string extension
+    = aarch64_get_extension_string_for_isa_flags (isa_flags);
 
   fprintf (file, "%*sselected tune = %s\n", indent, "", cpu->name);
-  fprintf (file, "%*sselected arch = %s", indent, "", arch->name);
-  aarch64_print_extension (file, isa_flags);
+  fprintf (file, "%*sselected arch = %s%s\n", indent, "",
+	   arch->name, extension.c_str ());
 }
 
 static GTY(()) tree aarch64_previous_fndecl;
@@ -10677,8 +10665,11 @@ aarch64_declare_function_name (FILE *stream, const char* name,
   const struct processor *this_arch
     = aarch64_get_arch (targ_options->x_explicit_arch);
 
-  asm_fprintf (asm_out_file, "\t.arch %s", this_arch->name);
-  aarch64_print_extension (asm_out_file, targ_options->x_aarch64_isa_flags);
+  unsigned long isa_flags = targ_options->x_aarch64_isa_flags;
+  std::string extension
+    = aarch64_get_extension_string_for_isa_flags (isa_flags);
+  asm_fprintf (asm_out_file, "\t.arch %s%s\n",
+	       this_arch->name, extension.c_str ());
 
   /* Print the cpu name we're tuning for in the comments, might be
      useful to readers of the generated asm.  */
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 1be78fc..1e5f5db 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -887,18 +887,18 @@ extern enum aarch64_code_model aarch64_cmodel;
   {"arch", "%{!march=*:%{!mcpu=*:-march=%(VALUE)}}" },	\
   {"cpu",  "%{!march=*:%{!mcpu=*:-mcpu=%(VALUE)}}" },
 
-#define BIG_LITTLE_SPEC \
-   " %{mcpu=*:-mcpu=%:rewrite_mcpu(%{mcpu=*:%*})}"
+#define MCPU_TO_MARCH_SPEC \
+   " %{mcpu=*:-march=%:rewrite_mcpu(%{mcpu=*:%*})}"
 
 extern const char *aarch64_rewrite_mcpu (int argc, const char **argv);
-#define BIG_LITTLE_CPU_SPEC_FUNCTIONS \
+#define MCPU_TO_MARCH_SPEC_FUNCTIONS \
   { "rewrite_mcpu", aarch64_rewrite_mcpu },
 
 #if defined(__aarch64__)
 extern const char *host_detect_local_cpu (int argc, const char **argv);
 # define EXTRA_SPEC_FUNCTIONS						\
   { "local_cpu_detect", host_detect_local_cpu },			\
-  BIG_LITTLE_CPU_SPEC_FUNCTIONS
+  MCPU_TO_MARCH_SPEC_FUNCTIONS
 
 # define MCPU_MTUNE_NATIVE_SPECS					\
    " %{march=native:%<march=native %:local_cpu_detect(arch)}"		\
@@ -906,11 +906,11 @@ extern const char *host_detect_local_cpu (int argc, const char **argv);
    " %{mtune=native:%<mtune=native %:local_cpu_detect(tune)}"
 #else
 # define MCPU_MTUNE_NATIVE_SPECS ""
-# define EXTRA_SPEC_FUNCTIONS BIG_LITTLE_CPU_SPEC_FUNCTIONS
+# define EXTRA_SPEC_FUNCTIONS MCPU_TO_MARCH_SPEC_FUNCTIONS
 #endif
 
 #define ASM_CPU_SPEC \
-   BIG_LITTLE_SPEC
+   MCPU_TO_MARCH_SPEC
 
 #define EXTRA_SPECS						\
   { "asm_cpu_spec",		ASM_CPU_SPEC }

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [AArch64] Break -mcpu tie between the compiler and assembler
  2015-08-19 15:43 [AArch64] Break -mcpu tie between the compiler and assembler James Greenhalgh
@ 2015-08-19 15:48 ` Andrew Pinski
  2015-08-19 15:50 ` Andrew Pinski
  1 sibling, 0 replies; 5+ messages in thread
From: Andrew Pinski @ 2015-08-19 15:48 UTC (permalink / raw)
  To: James Greenhalgh; +Cc: GCC Patches, Marcus Shawcroft, Richard Earnshaw

On Wed, Aug 19, 2015 at 11:39 PM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> This patch has been sitting in my tree for a while - it comes in handy
> when trying out bootstrap or test with -mcpu values like -mcpu=cortex-a72
> with a system assmebler which trails trunk binutils.
>
> Essentially, we rewrite -mcpu=foo to a -march flag providing the same
> architecture revision and set of optional architecture features. There
> is no reason we should ever need the assembler to see a CPU name, it
> should only be interested in the architecture variant.
>
> While we're there I've long found this function too fragile and hard
> to grok in C. So I've rewritten it in C++ to use std::string rather than
> raw C strings. Making this work with extension strings requires a slight
> refactor to the existing extension printing code to pull it across to
> somewhere common.
>
> Note that this also stops us from having to pick through a big.LITTLE
> target to find and separate the core names - we can just look up the
> architecture of the whole target and use that.
>
> The new function does leak the allocation of a C string to hold the
> result, but looking at gcc.c:getenv_spec_function and
> gcc.c:replace_extension_spec_func this is the usual thing to do.
>
> This has been through an aarch64-none-linux-gnu bootstrap and test run,
> configured with --with-cpu=cortex-a72 , which my system assembler does
> not understand.
>
> Ok?

I like this since this helps me not having to have a newer assembler
for -mcpu=thunderx.  Though I still need it for LSE support in the
assembler.  Has anyone thought about adding a configure test for v8.1
(LSE and other) support and disabling those extensions (yes this is
the same issue on x86_64 with AVX)?

Thanks,
Andrew

>
> Thanks,
> James
>
> ---
> 2015-08-19  James Greenhalgh  <james.greenhalgh@arm.com>
>
>         * common/config/aarch64/aarch64-common.c
>         (AARCH64_CPU_NAME_LENGTH): Delete.
>         (aarch64_option_extension): New.
>         (all_extensions): Likewise.
>         (processor_name_to_arch): Likewise.
>         (arch_to_arch_name): Likewise.
>         (all_cores): New.
>         (all_architectures): Likewise.
>         (aarch64_get_extension_string_for_isa_flags): Likewise.
>         (aarch64_rewrite_selected_cpu): Change to rewrite CPU names to
>         architecture names.
>         * config/aarch64/aarch64-protos.h
>         (aarch64_get_extension_string_for_isa_flags): New.
>         * config/aarch64/aarch64.c (aarch64_print_extension): Delete.
>         (aarch64_option_print): Get the string to print from
>         aarch64_get_extension_string_for_isa_flags.
>         (aarch64_declare_function_name): Likewise.
>         * config/aarch64/aarch64.h (BIG_LITTLE_SPEC): Rename to...
>         (MCPU_TO_MARCH_SPEC): This.
>         (ASM_CPU_SPEC): Use it.
>         (BIG_LITTLE_SPEC_FUNCTIONS): Rename to...
>         (MCPU_TO_MARCH_SPEC_FUNCTIONS): ...This.
>         (EXTRA_SPEC_FUNCTIONS): Use it.
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [AArch64] Break -mcpu tie between the compiler and assembler
  2015-08-19 15:43 [AArch64] Break -mcpu tie between the compiler and assembler James Greenhalgh
  2015-08-19 15:48 ` Andrew Pinski
@ 2015-08-19 15:50 ` Andrew Pinski
  2015-08-20  8:16   ` James Greenhalgh
  1 sibling, 1 reply; 5+ messages in thread
From: Andrew Pinski @ 2015-08-19 15:50 UTC (permalink / raw)
  To: James Greenhalgh; +Cc: GCC Patches, Marcus Shawcroft, Richard Earnshaw

On Wed, Aug 19, 2015 at 11:39 PM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> This patch has been sitting in my tree for a while - it comes in handy
> when trying out bootstrap or test with -mcpu values like -mcpu=cortex-a72
> with a system assmebler which trails trunk binutils.
>
> Essentially, we rewrite -mcpu=foo to a -march flag providing the same
> architecture revision and set of optional architecture features. There
> is no reason we should ever need the assembler to see a CPU name, it
> should only be interested in the architecture variant.
>
> While we're there I've long found this function too fragile and hard
> to grok in C. So I've rewritten it in C++ to use std::string rather than
> raw C strings. Making this work with extension strings requires a slight
> refactor to the existing extension printing code to pull it across to
> somewhere common.
>
> Note that this also stops us from having to pick through a big.LITTLE
> target to find and separate the core names - we can just look up the
> architecture of the whole target and use that.
>
> The new function does leak the allocation of a C string to hold the
> result, but looking at gcc.c:getenv_spec_function and
> gcc.c:replace_extension_spec_func this is the usual thing to do.
>
> This has been through an aarch64-none-linux-gnu bootstrap and test run,
> configured with --with-cpu=cortex-a72 , which my system assembler does
> not understand.
>
> Ok?


+     modified string, which seems much worse!  */
+  char *output = (char*) xmalloc (sizeof (*output)
+  * (outstr.length () + 1));
+  strcpy (output, outstr.c_str ());

Why not just:
char *output = xstrdup (outstr.c_str ());

Or at least use XNEWVEC instead of xmalloc with a cast?

Thanks,
Andrew Pinski

>
> Thanks,
> James
>
> ---
> 2015-08-19  James Greenhalgh  <james.greenhalgh@arm.com>
>
>         * common/config/aarch64/aarch64-common.c
>         (AARCH64_CPU_NAME_LENGTH): Delete.
>         (aarch64_option_extension): New.
>         (all_extensions): Likewise.
>         (processor_name_to_arch): Likewise.
>         (arch_to_arch_name): Likewise.
>         (all_cores): New.
>         (all_architectures): Likewise.
>         (aarch64_get_extension_string_for_isa_flags): Likewise.
>         (aarch64_rewrite_selected_cpu): Change to rewrite CPU names to
>         architecture names.
>         * config/aarch64/aarch64-protos.h
>         (aarch64_get_extension_string_for_isa_flags): New.
>         * config/aarch64/aarch64.c (aarch64_print_extension): Delete.
>         (aarch64_option_print): Get the string to print from
>         aarch64_get_extension_string_for_isa_flags.
>         (aarch64_declare_function_name): Likewise.
>         * config/aarch64/aarch64.h (BIG_LITTLE_SPEC): Rename to...
>         (MCPU_TO_MARCH_SPEC): This.
>         (ASM_CPU_SPEC): Use it.
>         (BIG_LITTLE_SPEC_FUNCTIONS): Rename to...
>         (MCPU_TO_MARCH_SPEC_FUNCTIONS): ...This.
>         (EXTRA_SPEC_FUNCTIONS): Use it.
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [AArch64] Break -mcpu tie between the compiler and assembler
  2015-08-19 15:50 ` Andrew Pinski
@ 2015-08-20  8:16   ` James Greenhalgh
  2015-08-20  9:11     ` Marcus Shawcroft
  0 siblings, 1 reply; 5+ messages in thread
From: James Greenhalgh @ 2015-08-20  8:16 UTC (permalink / raw)
  To: gcc-patches; +Cc: marcus.shawcroft, richard.earnshaw, pinskia

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


On Wed, Aug 19, 2015 at 04:48:11PM +0100, Andrew Pinski wrote:
> On Wed, Aug 19, 2015 at 11:39 PM, James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
> >
> > Hi,
> >
> > This patch has been sitting in my tree for a while - it comes in handy
> > when trying out bootstrap or test with -mcpu values like -mcpu=cortex-a72
> > with a system assmebler which trails trunk binutils.
> >
> > Essentially, we rewrite -mcpu=foo to a -march flag providing the same
> > architecture revision and set of optional architecture features. There
> > is no reason we should ever need the assembler to see a CPU name, it
> > should only be interested in the architecture variant.
> >
> > While we're there I've long found this function too fragile and hard
> > to grok in C. So I've rewritten it in C++ to use std::string rather than
> > raw C strings. Making this work with extension strings requires a slight
> > refactor to the existing extension printing code to pull it across to
> > somewhere common.
> >
> > Note that this also stops us from having to pick through a big.LITTLE
> > target to find and separate the core names - we can just look up the
> > architecture of the whole target and use that.
> >
> > The new function does leak the allocation of a C string to hold the
> > result, but looking at gcc.c:getenv_spec_function and
> > gcc.c:replace_extension_spec_func this is the usual thing to do.
> >
> > This has been through an aarch64-none-linux-gnu bootstrap and test run,
> > configured with --with-cpu=cortex-a72 , which my system assembler does
> > not understand.
> >
> > Ok?
>
>
> +     modified string, which seems much worse!  */
> +  char *output = (char*) xmalloc (sizeof (*output)
> +  * (outstr.length () + 1));
> +  strcpy (output, outstr.c_str ());
>
> Why not just:
> char *output = xstrdup (outstr.c_str ());
>
> Or at least use XNEWVEC instead of xmalloc with a cast?

Makes sense to me, patch updated along those lines.

OK?

Thanks,
James

---
2015-08-19  James Greenhalgh  <james.greenhalgh@arm.com>

	* common/config/aarch64/aarch64-common.c
	(AARCH64_CPU_NAME_LENGTH): Delete.
	(aarch64_option_extension): New.
	(all_extensions): Likewise.
	(processor_name_to_arch): Likewise.
	(arch_to_arch_name): Likewise.
	(all_cores): New.
	(all_architectures): Likewise.
	(aarch64_get_extension_string_for_isa_flags): Likewise.
	(aarch64_rewrite_selected_cpu): Change to rewrite CPU names to
	architecture names.
	* config/aarch64/aarch64-protos.h
	(aarch64_get_extension_string_for_isa_flags): New.
	* config/aarch64/aarch64.c (aarch64_print_extension): Delete.
	(aarch64_option_print): Get the string to print from
	aarch64_get_extension_string_for_isa_flags.
	(aarch64_declare_function_name): Likewise.
	* config/aarch64/aarch64.h (BIG_LITTLE_SPEC): Rename to...
	(MCPU_TO_MARCH_SPEC): This.
	(ASM_CPU_SPEC): Use it.
	(BIG_LITTLE_SPEC_FUNCTIONS): Rename to...
	(MCPU_TO_MARCH_SPEC_FUNCTIONS): ...This.
	(EXTRA_SPEC_FUNCTIONS): Use it.

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 726c625..07c6bba 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -27,7 +27,7 @@
 #include "common/common-target-def.h"
 #include "opts.h"
 #include "flags.h"
-#include "errors.h"
+#include "diagnostic.h"
 
 #ifdef  TARGET_BIG_ENDIAN_DEFAULT
 #undef  TARGET_DEFAULT_TARGET_FLAGS
@@ -107,36 +107,134 @@ aarch64_handle_option (struct gcc_options *opts,
 
 struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
 
-#define AARCH64_CPU_NAME_LENGTH 128
+/* An ISA extension in the co-processor and main instruction set space.  */
+struct aarch64_option_extension
+{
+  const char *const name;
+  const unsigned long flags_on;
+  const unsigned long flags_off;
+};
+
+/* ISA extensions in AArch64.  */
+static const struct aarch64_option_extension all_extensions[] =
+{
+#define AARCH64_OPT_EXTENSION(NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
+  {NAME, FLAGS_ON, FLAGS_OFF},
+#include "config/aarch64/aarch64-option-extensions.def"
+#undef AARCH64_OPT_EXTENSION
+  {NULL, 0, 0}
+};
+
+struct processor_name_to_arch
+{
+  const std::string processor_name;
+  const enum aarch64_arch arch;
+  const unsigned long flags;
+};
+
+struct arch_to_arch_name
+{
+  const enum aarch64_arch arch;
+  const std::string arch_name;
+};
+
+/* Map processor names to the architecture revision they implement and
+   the default set of architectural feature flags they support.  */
+static const struct processor_name_to_arch all_cores[] =
+{
+#define AARCH64_CORE(NAME, X, IDENT, ARCH_IDENT, FLAGS, COSTS, IMP, PART) \
+  {NAME, AARCH64_ARCH_##ARCH_IDENT, FLAGS},
+#include "config/aarch64/aarch64-cores.def"
+#undef AARCH64_CORE
+  {"generic", AARCH64_ARCH_8A, AARCH64_FL_FOR_ARCH8},
+  {"", aarch64_no_arch, 0}
+};
+
+/* Map architecture revisions to their string representation.  */
+static const struct arch_to_arch_name all_architectures[] =
+{
+#define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH, FLAGS) \
+  {AARCH64_ARCH_##ARCH_IDENT, NAME},
+#include "config/aarch64/aarch64-arches.def"
+#undef AARCH64_ARCH
+  {aarch64_no_arch, ""}
+};
+
+/* Return a string representation of ISA_FLAGS.  */
+
+std::string
+aarch64_get_extension_string_for_isa_flags (unsigned long isa_flags)
+{
+  const struct aarch64_option_extension *opt = NULL;
+  std::string outstr = "";
+
+  for (opt = all_extensions; opt->name != NULL; opt++)
+    if ((isa_flags & opt->flags_on) == opt->flags_on)
+      {
+	outstr += "+";
+	outstr += opt->name;
+      }
+  return outstr;
+}
 
-/* Truncate NAME at the first '.' character seen up to the first '+'
-   or return NAME unmodified.  */
+/* Attempt to rewrite NAME, which has been passed on the command line
+   as a -mcpu option to an equivalent -march value.  If we can do so,
+   return the new string, otherwise return an error.  */
 
 const char *
 aarch64_rewrite_selected_cpu (const char *name)
 {
-  static char output_buf[AARCH64_CPU_NAME_LENGTH + 1] = {0};
-  const char *bL_sep;
-  const char *feats;
-  size_t pref_size;
-  size_t feat_size;
-
-  bL_sep = strchr (name, '.');
-  if (!bL_sep)
-    return name;
+  std::string original_string (name);
+  std::string extensions;
+  std::string processor;
+  size_t extension_pos = original_string.find_first_of ('+');
 
-  feats = strchr (name, '+');
-  feat_size = feats ? strnlen (feats, AARCH64_CPU_NAME_LENGTH) : 0;
-  pref_size = bL_sep - name;
+  /* Strip and save the extension string.  */
+  if (extension_pos != std::string::npos)
+    {
+      processor = original_string.substr (0, extension_pos);
+      extensions = original_string.substr (extension_pos,
+					std::string::npos);
+    }
+  else
+    {
+      /* No extensions.  */
+      processor = original_string;
+    }
 
-  if ((feat_size + pref_size) > AARCH64_CPU_NAME_LENGTH)
-    internal_error ("-mcpu string too large");
+  const struct processor_name_to_arch* p_to_a;
+  for (p_to_a = all_cores;
+       p_to_a->arch != aarch64_no_arch;
+       p_to_a++)
+    {
+      if (p_to_a->processor_name == processor)
+	break;
+    }
 
-  strncpy (output_buf, name, pref_size);
-  if (feats)
-    strncpy (output_buf + pref_size, feats, feat_size);
+  const struct arch_to_arch_name* a_to_an;
+  for (a_to_an = all_architectures;
+       a_to_an->arch != aarch64_no_arch;
+       a_to_an++)
+    {
+      if (a_to_an->arch == p_to_a->arch)
+	break;
+    }
 
-  return output_buf;
+  /* We couldn't find that proceesor name, or the processor name we
+     found does not map to an architecture we understand.  */
+  if (p_to_a->arch == aarch64_no_arch
+      || a_to_an->arch == aarch64_no_arch)
+    fatal_error (input_location, "unknown value %qs for -mcpu", name);
+
+  std::string outstr = a_to_an->arch_name
+	+ aarch64_get_extension_string_for_isa_flags (p_to_a->flags)
+	+ extensions;
+
+  /* We are going to memory leak here, nobody elsewhere
+     in the callchain is going to clean up after us.  The alternative is
+     to allocate a static buffer, and assert that it is big enough for our
+     modified string, which seems much worse!  */
+  return xstrdup (outstr.c_str ());
 }
 
 /* Called by the driver to rewrite a name passed to the -mcpu
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 0b09d49..06002d8 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -310,6 +310,7 @@ rtx aarch64_simd_gen_const_vector_dup (machine_mode, int);
 bool aarch64_simd_mem_operand_p (rtx);
 rtx aarch64_simd_vect_par_cnst_half (machine_mode, bool);
 rtx aarch64_tls_get_addr (void);
+std::string aarch64_get_extension_string_for_isa_flags (unsigned long);
 tree aarch64_fold_builtin (tree, int, tree *, bool);
 unsigned aarch64_dbx_register_number (unsigned);
 unsigned aarch64_trampoline_size (void);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index aa268ae..db316a0 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7904,20 +7904,6 @@ initialize_aarch64_code_model (struct gcc_options *opts)
      aarch64_cmodel = opts->x_aarch64_cmodel_var;
 }
 
-/* Print to F the architecture features specified by ISA_FLAGS.  */
-
-static void
-aarch64_print_extension (FILE *f, unsigned long isa_flags)
-{
-  const struct aarch64_option_extension *opt = NULL;
-
-  for (opt = all_extensions; opt->name != NULL; opt++)
-    if ((isa_flags & opt->flags_on) == opt->flags_on)
-      asm_fprintf (f, "+%s", opt->name);
-
-  asm_fprintf (f, "\n");
-}
-
 /* Implement TARGET_OPTION_SAVE.  */
 
 static void
@@ -7950,10 +7936,12 @@ aarch64_option_print (FILE *file, int indent, struct cl_target_option *ptr)
     = aarch64_get_tune_cpu (ptr->x_explicit_tune_core);
   unsigned long isa_flags = ptr->x_aarch64_isa_flags;
   const struct processor *arch = aarch64_get_arch (ptr->x_explicit_arch);
+  std::string extension
+    = aarch64_get_extension_string_for_isa_flags (isa_flags);
 
   fprintf (file, "%*sselected tune = %s\n", indent, "", cpu->name);
-  fprintf (file, "%*sselected arch = %s", indent, "", arch->name);
-  aarch64_print_extension (file, isa_flags);
+  fprintf (file, "%*sselected arch = %s%s\n", indent, "",
+	   arch->name, extension.c_str ());
 }
 
 static GTY(()) tree aarch64_previous_fndecl;
@@ -10677,8 +10665,11 @@ aarch64_declare_function_name (FILE *stream, const char* name,
   const struct processor *this_arch
     = aarch64_get_arch (targ_options->x_explicit_arch);
 
-  asm_fprintf (asm_out_file, "\t.arch %s", this_arch->name);
-  aarch64_print_extension (asm_out_file, targ_options->x_aarch64_isa_flags);
+  unsigned long isa_flags = targ_options->x_aarch64_isa_flags;
+  std::string extension
+    = aarch64_get_extension_string_for_isa_flags (isa_flags);
+  asm_fprintf (asm_out_file, "\t.arch %s%s\n",
+	       this_arch->name, extension.c_str ());
 
   /* Print the cpu name we're tuning for in the comments, might be
      useful to readers of the generated asm.  */
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 1be78fc..1e5f5db 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -887,18 +887,18 @@ extern enum aarch64_code_model aarch64_cmodel;
   {"arch", "%{!march=*:%{!mcpu=*:-march=%(VALUE)}}" },	\
   {"cpu",  "%{!march=*:%{!mcpu=*:-mcpu=%(VALUE)}}" },
 
-#define BIG_LITTLE_SPEC \
-   " %{mcpu=*:-mcpu=%:rewrite_mcpu(%{mcpu=*:%*})}"
+#define MCPU_TO_MARCH_SPEC \
+   " %{mcpu=*:-march=%:rewrite_mcpu(%{mcpu=*:%*})}"
 
 extern const char *aarch64_rewrite_mcpu (int argc, const char **argv);
-#define BIG_LITTLE_CPU_SPEC_FUNCTIONS \
+#define MCPU_TO_MARCH_SPEC_FUNCTIONS \
   { "rewrite_mcpu", aarch64_rewrite_mcpu },
 
 #if defined(__aarch64__)
 extern const char *host_detect_local_cpu (int argc, const char **argv);
 # define EXTRA_SPEC_FUNCTIONS						\
   { "local_cpu_detect", host_detect_local_cpu },			\
-  BIG_LITTLE_CPU_SPEC_FUNCTIONS
+  MCPU_TO_MARCH_SPEC_FUNCTIONS
 
 # define MCPU_MTUNE_NATIVE_SPECS					\
    " %{march=native:%<march=native %:local_cpu_detect(arch)}"		\
@@ -906,11 +906,11 @@ extern const char *host_detect_local_cpu (int argc, const char **argv);
    " %{mtune=native:%<mtune=native %:local_cpu_detect(tune)}"
 #else
 # define MCPU_MTUNE_NATIVE_SPECS ""
-# define EXTRA_SPEC_FUNCTIONS BIG_LITTLE_CPU_SPEC_FUNCTIONS
+# define EXTRA_SPEC_FUNCTIONS MCPU_TO_MARCH_SPEC_FUNCTIONS
 #endif
 
 #define ASM_CPU_SPEC \
-   BIG_LITTLE_SPEC
+   MCPU_TO_MARCH_SPEC
 
 #define EXTRA_SPECS						\
   { "asm_cpu_spec",		ASM_CPU_SPEC }

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [AArch64] Break -mcpu tie between the compiler and assembler
  2015-08-20  8:16   ` James Greenhalgh
@ 2015-08-20  9:11     ` Marcus Shawcroft
  0 siblings, 0 replies; 5+ messages in thread
From: Marcus Shawcroft @ 2015-08-20  9:11 UTC (permalink / raw)
  To: James Greenhalgh; +Cc: gcc-patches, Marcus Shawcroft, Richard Earnshaw, pinskia

On 20 August 2015 at 09:15, James Greenhalgh <james.greenhalgh@arm.com> wrote:

> 2015-08-19  James Greenhalgh  <james.greenhalgh@arm.com>
>
>         * common/config/aarch64/aarch64-common.c
>         (AARCH64_CPU_NAME_LENGTH): Delete.
>         (aarch64_option_extension): New.
>         (all_extensions): Likewise.
>         (processor_name_to_arch): Likewise.
>         (arch_to_arch_name): Likewise.
>         (all_cores): New.
>         (all_architectures): Likewise.
>         (aarch64_get_extension_string_for_isa_flags): Likewise.
>         (aarch64_rewrite_selected_cpu): Change to rewrite CPU names to
>         architecture names.
>         * config/aarch64/aarch64-protos.h
>         (aarch64_get_extension_string_for_isa_flags): New.
>         * config/aarch64/aarch64.c (aarch64_print_extension): Delete.
>         (aarch64_option_print): Get the string to print from
>         aarch64_get_extension_string_for_isa_flags.
>         (aarch64_declare_function_name): Likewise.
>         * config/aarch64/aarch64.h (BIG_LITTLE_SPEC): Rename to...
>         (MCPU_TO_MARCH_SPEC): This.
>         (ASM_CPU_SPEC): Use it.
>         (BIG_LITTLE_SPEC_FUNCTIONS): Rename to...
>         (MCPU_TO_MARCH_SPEC_FUNCTIONS): ...This.
>         (EXTRA_SPEC_FUNCTIONS): Use it.

OK /Marcus

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-08-20  9:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-19 15:43 [AArch64] Break -mcpu tie between the compiler and assembler James Greenhalgh
2015-08-19 15:48 ` Andrew Pinski
2015-08-19 15:50 ` Andrew Pinski
2015-08-20  8:16   ` James Greenhalgh
2015-08-20  9:11     ` Marcus Shawcroft

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).