[PATCH][AArch64] Only update assembler .arch directive when necessary

[PATCH][AArch64] Only update assembler .arch directive when necessary

[Kyrill Tkachov]

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

Hi all,

As part of the target attributes and pragmas support for GCC 6 I changed the aarch64 port
to emit a .arch assembly directive for each function that describes the architectural features
used by that function.  This is a change from GCC 5 behaviour where we output a single .arch directive
at the beginning of the assembly file corresponding to architectural features given on the command line.

We need that change to accommodate use cases where a user tags a single function with a target attribute
that changes the arch features from those given on the command line and then proceeds to use an inline
assembly instruction that needs those new features.  We need to emit the new architecture state for that
function as a .arch directive to prevent the assembler from rejecting the assembly.

Unfortunately, the new behaviour has caused some headaches when building the Linux kernel.
There, they have a header called lse.h that is used to access some armv8.1-a-specific inline assembly
(using the "lse" architecture feature in particular)
This header has the line:
__asm__(".arch_extension        lse");
to tell the assembler to accept LSE instructions.
This header is included in various .c files so the .arch_extension appears at the beginning of the
assembly file.

But since we now emit a .arch directive for every function, we effectively reset the architectural state
on each function, rendering that .arch_extension ineffective.

GCC is arguably right in emitting the .arch directive on each function, but I'd like to make life easier
for the kernel folk so this patch helps with that.

With this patch we go back to emitting the .arch directive at the top of the assembly file, same in GCC 5.
We will still emit per-function .arch directives but only if they differ from the last .arch directive
that we emitted i.e. when the architecture features were changed due to a target attribute or pragma.
For code that doesn't make use of target attributes or pragmas the behaviour of GCC 5 is thus preserved
and we still get the correct .arch updates when needed.

The TARGET_ASM_FILE_START hook implementation is brought back after I deleted it during the target
attributes work last August and is used again to output the .arch directive at the top of the file.
We keep track of the architecture and the architecture features we last output in the asm directive
in a new static variable (yuck, but unavoidable IMO) and output the new .arch directive only if
what we want to output is different from the previously output directive.

This still allows the kernel to do naughty things with .arch_extension directives in header files,
but this usecase is one where they really know what they're doing and don't want the assembler
to get in their way.

Bootstrapped and tested on aarch64-none-linux-gnu.
With this patch I managed to build a recent allyesconfig Linux kernel where before the build would fail
when assembling the LSE instructions.

Ok for trunk?

Thanks,
Kyrill

2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
     New struct definition.
     (aarch64_previous_asm_output): New variable.
     (aarch64_declare_function_name): Only output .arch assembler
     directive if it will be different from the previously output
     directive.
     (aarch64_start_file): New function.
     (TARGET_ASM_FILE_START): Define.

2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
     Delete unneeded -save-temps.
     * gcc.target/aarch64/assembler_arch_7.c: Likewise.
     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
     .arch armv8-a\n.
     * gcc.target/aarch64/assembler_arch_1.c: New test.

[-- Attachment #2: aarch64-asm.patch --]
[-- Type: text/x-patch, Size: 5968 bytes --]

commit 2df0f24332e316b8d18d4571438f76726a0326e7
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Jan 27 12:54:54 2016 +0000

    [AArch64] Only update assembler .arch directive when necessary

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5ca2ae8..0751440 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
    return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
 }
 
+struct aarch64_output_asm_info
+{
+  const struct processor *arch;
+  const struct processor *cpu;
+  unsigned long isa_flags;
+};
+
+/* A record of the last architecture state that we output in assembly
+   i.e. the .arch directive.  */
+static struct aarch64_output_asm_info aarch64_previous_asm_output;
+
 /* Implement ASM_DECLARE_FUNCTION_NAME.  Output the ISA features used
    by the function fndecl.  */
 
@@ -11183,25 +11194,60 @@ aarch64_declare_function_name (FILE *stream, const char* name,
     = aarch64_get_arch (targ_options->x_explicit_arch);
 
   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 ());
+  /* Only update the assembler .arch state if we are changing to a
+     distinct architecture state.  */
+  if (this_arch != aarch64_previous_asm_output.arch
+      || isa_flags != aarch64_previous_asm_output.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 ());
 
+      aarch64_previous_asm_output.arch = this_arch;
+      aarch64_previous_asm_output.isa_flags = isa_flags;
+    }
   /* Print the cpu name we're tuning for in the comments, might be
      useful to readers of the generated asm.  */
 
   const struct processor *this_tune
     = aarch64_get_tune_cpu (targ_options->x_explicit_tune_core);
 
-  asm_fprintf (asm_out_file, "\t" ASM_COMMENT_START ".tune %s\n",
-	       this_tune->name);
+  if (flag_debug_asm && this_tune != aarch64_previous_asm_output.cpu)
+    {
+      asm_fprintf (asm_out_file, "\t" ASM_COMMENT_START ".tune %s\n",
+		   this_tune->name);
+      aarch64_previous_asm_output.cpu = this_tune;
+    }
 
   /* Don't forget the type directive for ELF.  */
   ASM_OUTPUT_TYPE_DIRECTIVE (stream, name, "function");
   ASM_OUTPUT_LABEL (stream, name);
 }
 
+/* Implements TARGET_ASM_FILE_START.  Output the assembly header.  */
+
+static void
+aarch64_start_file (void)
+{
+  struct cl_target_option *default_options
+    = TREE_TARGET_OPTION (target_option_default_node);
+
+  const struct processor *default_arch
+    = aarch64_get_arch (default_options->x_explicit_arch);
+  unsigned long default_isa_flags = default_options->x_aarch64_isa_flags;
+  std::string extension
+    = aarch64_get_extension_string_for_isa_flags (default_isa_flags);
+
+   asm_fprintf (asm_out_file, "\t.arch %s%s\n", default_arch->name,
+		extension.c_str ());
+   aarch64_previous_asm_output.arch = default_arch;
+   aarch64_previous_asm_output.isa_flags = default_isa_flags;
+   aarch64_previous_asm_output.cpu = NULL;
+
+   default_file_start ();
+}
+
 /* Emit load exclusive.  */
 
 static void
@@ -13935,6 +13981,9 @@ aarch64_optab_supported_p (int op, machine_mode, machine_mode,
 #define TARGET_ASM_CAN_OUTPUT_MI_THUNK \
   hook_bool_const_tree_hwi_hwi_const_tree_true
 
+#undef TARGET_ASM_FILE_START
+#define TARGET_ASM_FILE_START aarch64_start_file
+
 #undef TARGET_ASM_OUTPUT_MI_THUNK
 #define TARGET_ASM_OUTPUT_MI_THUNK aarch64_output_mi_thunk
 
diff --git a/gcc/testsuite/gcc.target/aarch64/assembler_arch_1.c b/gcc/testsuite/gcc.target/aarch64/assembler_arch_1.c
new file mode 100644
index 0000000..901e50a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/assembler_arch_1.c
@@ -0,0 +1,20 @@
+/* { dg-do assemble } */
+/* { dg-options "-march=armv8-a" } */
+
+/* Make sure that the function header in assembly doesn't override
+   user asm arch_extension directives.  */
+
+__asm__ (".arch_extension lse");
+
+void
+foo (int i, int *v)
+{
+  register int w0 asm ("w0") = i;
+  register int *x1 asm ("x1") = v;
+
+  asm volatile (
+  "\tstset   %w[i], %[v]\n"
+  : [i] "+r" (w0), [v] "+Q" (v)
+  : "r" (x1)
+  : "x30");
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_1.c b/gcc/testsuite/gcc.target/aarch64/target_attr_1.c
index 852ce1e..0527d0c 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_1.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -mcpu=thunderx -save-temps" } */
+/* { dg-options "-O2 -mcpu=thunderx -dA" } */
 
 /* Test that cpu attribute overrides the command-line -mcpu.  */
 
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_15.c b/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
index 02091c6..f72bec8 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
@@ -10,6 +10,4 @@ foo (int a)
   return a + 1;
 }
 
-/* { dg-final { scan-assembler-not "\\+fp" } } */
-/* { dg-final { scan-assembler-not "\\+crypto" } } */
-/* { dg-final { scan-assembler-not "\\+simd" } } */
+/* { dg-final { scan-assembler-times "\\.arch armv8-a\n" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_7.c b/gcc/testsuite/gcc.target/aarch64/target_attr_7.c
index 32a8403..818d327 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_7.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_7.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -mcpu=thunderx -save-temps" } */
+/* { dg-options "-O2 -mcpu=thunderx -dA" } */
 
 /* Make sure that #pragma overrides command line option and
    target attribute overrides the pragma.  */

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Andrew Pinski]

On Thu, Feb 4, 2016 at 5:50 AM, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi all,
>
> As part of the target attributes and pragmas support for GCC 6 I changed the
> aarch64 port
> to emit a .arch assembly directive for each function that describes the
> architectural features
> used by that function.  This is a change from GCC 5 behaviour where we
> output a single .arch directive
> at the beginning of the assembly file corresponding to architectural
> features given on the command line.
>
> We need that change to accommodate use cases where a user tags a single
> function with a target attribute
> that changes the arch features from those given on the command line and then
> proceeds to use an inline
> assembly instruction that needs those new features.  We need to emit the new
> architecture state for that
> function as a .arch directive to prevent the assembler from rejecting the
> assembly.
>
> Unfortunately, the new behaviour has caused some headaches when building the
> Linux kernel.
> There, they have a header called lse.h that is used to access some
> armv8.1-a-specific inline assembly
> (using the "lse" architecture feature in particular)
> This header has the line:
> __asm__(".arch_extension        lse");
> to tell the assembler to accept LSE instructions.
> This header is included in various .c files so the .arch_extension appears
> at the beginning of the
> assembly file.
>
> But since we now emit a .arch directive for every function, we effectively
> reset the architectural state
> on each function, rendering that .arch_extension ineffective.
>
> GCC is arguably right in emitting the .arch directive on each function, but
> I'd like to make life easier
> for the kernel folk so this patch helps with that.

I had suggested a change to the Linux kernel already to fix this issue:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395191.html

Thanks,
Andrew Pinski

>
> With this patch we go back to emitting the .arch directive at the top of the
> assembly file, same in GCC 5.
> We will still emit per-function .arch directives but only if they differ
> from the last .arch directive
> that we emitted i.e. when the architecture features were changed due to a
> target attribute or pragma.
> For code that doesn't make use of target attributes or pragmas the behaviour
> of GCC 5 is thus preserved
> and we still get the correct .arch updates when needed.
>
> The TARGET_ASM_FILE_START hook implementation is brought back after I
> deleted it during the target
> attributes work last August and is used again to output the .arch directive
> at the top of the file.
> We keep track of the architecture and the architecture features we last
> output in the asm directive
> in a new static variable (yuck, but unavoidable IMO) and output the new
> .arch directive only if
> what we want to output is different from the previously output directive.
>
> This still allows the kernel to do naughty things with .arch_extension
> directives in header files,
> but this usecase is one where they really know what they're doing and don't
> want the assembler
> to get in their way.
>
> Bootstrapped and tested on aarch64-none-linux-gnu.
> With this patch I managed to build a recent allyesconfig Linux kernel where
> before the build would fail
> when assembling the LSE instructions.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>     New struct definition.
>     (aarch64_previous_asm_output): New variable.
>     (aarch64_declare_function_name): Only output .arch assembler
>     directive if it will be different from the previously output
>     directive.
>     (aarch64_start_file): New function.
>     (TARGET_ASM_FILE_START): Define.
>
> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>     Delete unneeded -save-temps.
>     * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>     .arch armv8-a\n.
>     * gcc.target/aarch64/assembler_arch_1.c: New test.

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Kyrill Tkachov]

On 04/02/16 17:12, Andrew Pinski wrote:
> On Thu, Feb 4, 2016 at 5:50 AM, Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
>> Hi all,
>>
>> As part of the target attributes and pragmas support for GCC 6 I changed the
>> aarch64 port
>> to emit a .arch assembly directive for each function that describes the
>> architectural features
>> used by that function.  This is a change from GCC 5 behaviour where we
>> output a single .arch directive
>> at the beginning of the assembly file corresponding to architectural
>> features given on the command line.
>>
>> We need that change to accommodate use cases where a user tags a single
>> function with a target attribute
>> that changes the arch features from those given on the command line and then
>> proceeds to use an inline
>> assembly instruction that needs those new features.  We need to emit the new
>> architecture state for that
>> function as a .arch directive to prevent the assembler from rejecting the
>> assembly.
>>
>> Unfortunately, the new behaviour has caused some headaches when building the
>> Linux kernel.
>> There, they have a header called lse.h that is used to access some
>> armv8.1-a-specific inline assembly
>> (using the "lse" architecture feature in particular)
>> This header has the line:
>> __asm__(".arch_extension        lse");
>> to tell the assembler to accept LSE instructions.
>> This header is included in various .c files so the .arch_extension appears
>> at the beginning of the
>> assembly file.
>>
>> But since we now emit a .arch directive for every function, we effectively
>> reset the architectural state
>> on each function, rendering that .arch_extension ineffective.
>>
>> GCC is arguably right in emitting the .arch directive on each function, but
>> I'd like to make life easier
>> for the kernel folk so this patch helps with that.
> I had suggested a change to the Linux kernel already to fix this issue:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/395191.html

Indeed, but I don't see this patch in the 4.3 or 4.4 Linux kernel
releases that I tried, so we'd still want GCC 6 to be able to compile those.

Kyrill

> Thanks,
> Andrew Pinski
>
>> With this patch we go back to emitting the .arch directive at the top of the
>> assembly file, same in GCC 5.
>> We will still emit per-function .arch directives but only if they differ
>> from the last .arch directive
>> that we emitted i.e. when the architecture features were changed due to a
>> target attribute or pragma.
>> For code that doesn't make use of target attributes or pragmas the behaviour
>> of GCC 5 is thus preserved
>> and we still get the correct .arch updates when needed.
>>
>> The TARGET_ASM_FILE_START hook implementation is brought back after I
>> deleted it during the target
>> attributes work last August and is used again to output the .arch directive
>> at the top of the file.
>> We keep track of the architecture and the architecture features we last
>> output in the asm directive
>> in a new static variable (yuck, but unavoidable IMO) and output the new
>> .arch directive only if
>> what we want to output is different from the previously output directive.
>>
>> This still allows the kernel to do naughty things with .arch_extension
>> directives in header files,
>> but this usecase is one where they really know what they're doing and don't
>> want the assembler
>> to get in their way.
>>
>> Bootstrapped and tested on aarch64-none-linux-gnu.
>> With this patch I managed to build a recent allyesconfig Linux kernel where
>> before the build would fail
>> when assembling the LSE instructions.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Kyrill
>>
>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>>      New struct definition.
>>      (aarch64_previous_asm_output): New variable.
>>      (aarch64_declare_function_name): Only output .arch assembler
>>      directive if it will be different from the previously output
>>      directive.
>>      (aarch64_start_file): New function.
>>      (TARGET_ASM_FILE_START): Define.
>>
>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>>      Delete unneeded -save-temps.
>>      * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>>      * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>      .arch armv8-a\n.
>>      * gcc.target/aarch64/assembler_arch_1.c: New test.

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[James Greenhalgh]

On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
> Hi all,
> 
> As part of the target attributes and pragmas support for GCC 6 I changed the
> aarch64 port to emit a .arch assembly directive for each function that
> describes the architectural features used by that function.  This is a change
> from GCC 5 behaviour where we output a single .arch directive at the
> beginning of the assembly file corresponding to architectural features given
> on the command line.
<snip>
> Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I managed
> to build a recent allyesconfig Linux kernel where before the build would fail
> when assembling the LSE instructions.
> 
> Ok for trunk?

One comment, that I'm willing to be convinced on...

> 
> Thanks,
> Kyrill
> 
> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>     New struct definition.
>     (aarch64_previous_asm_output): New variable.
>     (aarch64_declare_function_name): Only output .arch assembler
>     directive if it will be different from the previously output
>     directive.
>     (aarch64_start_file): New function.
>     (TARGET_ASM_FILE_START): Define.
> 
> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>     Delete unneeded -save-temps.
>     * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>     .arch armv8-a\n.
>     * gcc.target/aarch64/assembler_arch_1.c: New test.

> commit 2df0f24332e316b8d18d4571438f76726a0326e7
> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> Date:   Wed Jan 27 12:54:54 2016 +0000
> 
>     [AArch64] Only update assembler .arch directive when necessary
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 5ca2ae8..0751440 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
>     return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
>  }
>  
> +struct aarch64_output_asm_info
> +{
> +  const struct processor *arch;
> +  const struct processor *cpu;
> +  unsigned long isa_flags;

Why not just keep the last string you printed, and use a string compare
to decide whether to print or not? Sure we'll end up doing a bit more
work, but the logic becomes simpler to follow and we don't need to pass
around another struct...

Thanks,
James

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Kyrill Tkachov]

Hi James,

On 10/02/16 10:11, James Greenhalgh wrote:
> On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
>> Hi all,
>>
>> As part of the target attributes and pragmas support for GCC 6 I changed the
>> aarch64 port to emit a .arch assembly directive for each function that
>> describes the architectural features used by that function.  This is a change
>> from GCC 5 behaviour where we output a single .arch directive at the
>> beginning of the assembly file corresponding to architectural features given
>> on the command line.
> <snip>
>> Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I managed
>> to build a recent allyesconfig Linux kernel where before the build would fail
>> when assembling the LSE instructions.
>>
>> Ok for trunk?
> One comment, that I'm willing to be convinced on...
>
>> Thanks,
>> Kyrill
>>
>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>>      New struct definition.
>>      (aarch64_previous_asm_output): New variable.
>>      (aarch64_declare_function_name): Only output .arch assembler
>>      directive if it will be different from the previously output
>>      directive.
>>      (aarch64_start_file): New function.
>>      (TARGET_ASM_FILE_START): Define.
>>
>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>>      Delete unneeded -save-temps.
>>      * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>>      * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>      .arch armv8-a\n.
>>      * gcc.target/aarch64/assembler_arch_1.c: New test.
>> commit 2df0f24332e316b8d18d4571438f76726a0326e7
>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>> Date:   Wed Jan 27 12:54:54 2016 +0000
>>
>>      [AArch64] Only update assembler .arch directive when necessary
>>
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index 5ca2ae8..0751440 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
>>      return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
>>   }
>>   
>> +struct aarch64_output_asm_info
>> +{
>> +  const struct processor *arch;
>> +  const struct processor *cpu;
>> +  unsigned long isa_flags;
> Why not just keep the last string you printed, and use a string compare
> to decide whether to print or not? Sure we'll end up doing a bit more
> work, but the logic becomes simpler to follow and we don't need to pass
> around another struct...

I did do it this way to avoid a string comparison (I try to avoid
manual string manipulations where I can as they're so easy to get wrong)
though this isn't on any hot path.
We don't really pass the structure around anywhere, we just keep one
instance. We'd have to do the same with a string i.e. keep a string
object around that we'd strcpy (or C++ equivalent) a string to every time
we wanted to update it, so I thought this approach is cleaner as the
architecture features are already fully described by a pointer to
an element in the static constant all_architectures table and an
unsigned long holding the ISA flags.

If you insist I can change it to a string, but I personally don't
think it's worth it.

Thanks,
Kyrill


> Thanks,
> James
>
>

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[James Greenhalgh]

On Wed, Feb 10, 2016 at 10:32:16AM +0000, Kyrill Tkachov wrote:
> Hi James,
> 
> On 10/02/16 10:11, James Greenhalgh wrote:
> >On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
> >>Hi all,
> >>
> >>As part of the target attributes and pragmas support for GCC 6 I changed the
> >>aarch64 port to emit a .arch assembly directive for each function that
> >>describes the architectural features used by that function.  This is a change
> >>from GCC 5 behaviour where we output a single .arch directive at the
> >>beginning of the assembly file corresponding to architectural features given
> >>on the command line.
> ><snip>
> >>Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I managed
> >>to build a recent allyesconfig Linux kernel where before the build would fail
> >>when assembling the LSE instructions.
> >>
> >>Ok for trunk?
> >One comment, that I'm willing to be convinced on...
> >
> >>Thanks,
> >>Kyrill
> >>
> >>2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> >>
> >>     * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
> >>     New struct definition.
> >>     (aarch64_previous_asm_output): New variable.
> >>     (aarch64_declare_function_name): Only output .arch assembler
> >>     directive if it will be different from the previously output
> >>     directive.
> >>     (aarch64_start_file): New function.
> >>     (TARGET_ASM_FILE_START): Define.
> >>
> >>2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> >>
> >>     * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
> >>     Delete unneeded -save-temps.
> >>     * gcc.target/aarch64/assembler_arch_7.c: Likewise.
> >>     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
> >>     .arch armv8-a\n.
> >>     * gcc.target/aarch64/assembler_arch_1.c: New test.
> >>commit 2df0f24332e316b8d18d4571438f76726a0326e7
> >>Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> >>Date:   Wed Jan 27 12:54:54 2016 +0000
> >>
> >>     [AArch64] Only update assembler .arch directive when necessary
> >>
> >>diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >>index 5ca2ae8..0751440 100644
> >>--- a/gcc/config/aarch64/aarch64.c
> >>+++ b/gcc/config/aarch64/aarch64.c
> >>@@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
> >>     return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
> >>  }
> >>+struct aarch64_output_asm_info
> >>+{
> >>+  const struct processor *arch;
> >>+  const struct processor *cpu;
> >>+  unsigned long isa_flags;
> >Why not just keep the last string you printed, and use a string compare
> >to decide whether to print or not? Sure we'll end up doing a bit more
> >work, but the logic becomes simpler to follow and we don't need to pass
> >around another struct...
> 
> I did do it this way to avoid a string comparison (I try to avoid
> manual string manipulations where I can as they're so easy to get wrong)
> though this isn't on any hot path.
> We don't really pass the structure around anywhere, we just keep one
> instance. We'd have to do the same with a string i.e. keep a string
> object around that we'd strcpy (or C++ equivalent) a string to every time
> we wanted to update it, so I thought this approach is cleaner as the
> architecture features are already fully described by a pointer to
> an element in the static constant all_architectures table and an
> unsigned long holding the ISA flags.
> 
> If you insist I can change it to a string, but I personally don't
> think it's worth it.

Had you been working on a C string I probably wouldn't have noticed. But
you're already working with C++ strings in this function, so much of what
you are concerned about is straightforward.

I'd encourage you to try it using idiomatic string manipulation in C++, the
cleanup should be worth it.

Thanks,
James

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Kyrill Tkachov]

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


On 10/02/16 10:39, James Greenhalgh wrote:
> On Wed, Feb 10, 2016 at 10:32:16AM +0000, Kyrill Tkachov wrote:
>> Hi James,
>>
>> On 10/02/16 10:11, James Greenhalgh wrote:
>>> On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
>>>> Hi all,
>>>>
>>>> As part of the target attributes and pragmas support for GCC 6 I changed the
>>>> aarch64 port to emit a .arch assembly directive for each function that
>>>> describes the architectural features used by that function.  This is a change
>>> >from GCC 5 behaviour where we output a single .arch directive at the
>>>> beginning of the assembly file corresponding to architectural features given
>>>> on the command line.
>>> <snip>
>>>> Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I managed
>>>> to build a recent allyesconfig Linux kernel where before the build would fail
>>>> when assembling the LSE instructions.
>>>>
>>>> Ok for trunk?
>>> One comment, that I'm willing to be convinced on...
>>>
>>>> Thanks,
>>>> Kyrill
>>>>
>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>
>>>>      * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>>>>      New struct definition.
>>>>      (aarch64_previous_asm_output): New variable.
>>>>      (aarch64_declare_function_name): Only output .arch assembler
>>>>      directive if it will be different from the previously output
>>>>      directive.
>>>>      (aarch64_start_file): New function.
>>>>      (TARGET_ASM_FILE_START): Define.
>>>>
>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>
>>>>      * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>>>>      Delete unneeded -save-temps.
>>>>      * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>>>>      * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>>>      .arch armv8-a\n.
>>>>      * gcc.target/aarch64/assembler_arch_1.c: New test.
>>>> commit 2df0f24332e316b8d18d4571438f76726a0326e7
>>>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>>> Date:   Wed Jan 27 12:54:54 2016 +0000
>>>>
>>>>      [AArch64] Only update assembler .arch directive when necessary
>>>>
>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>>> index 5ca2ae8..0751440 100644
>>>> --- a/gcc/config/aarch64/aarch64.c
>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>> @@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
>>>>      return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
>>>>   }
>>>> +struct aarch64_output_asm_info
>>>> +{
>>>> +  const struct processor *arch;
>>>> +  const struct processor *cpu;
>>>> +  unsigned long isa_flags;
>>> Why not just keep the last string you printed, and use a string compare
>>> to decide whether to print or not? Sure we'll end up doing a bit more
>>> work, but the logic becomes simpler to follow and we don't need to pass
>>> around another struct...
>> I did do it this way to avoid a string comparison (I try to avoid
>> manual string manipulations where I can as they're so easy to get wrong)
>> though this isn't on any hot path.
>> We don't really pass the structure around anywhere, we just keep one
>> instance. We'd have to do the same with a string i.e. keep a string
>> object around that we'd strcpy (or C++ equivalent) a string to every time
>> we wanted to update it, so I thought this approach is cleaner as the
>> architecture features are already fully described by a pointer to
>> an element in the static constant all_architectures table and an
>> unsigned long holding the ISA flags.
>>
>> If you insist I can change it to a string, but I personally don't
>> think it's worth it.
> Had you been working on a C string I probably wouldn't have noticed. But
> you're already working with C++ strings in this function, so much of what
> you are concerned about is straightforward.
>
> I'd encourage you to try it using idiomatic string manipulation in C++, the
> cleanup should be worth it.

Ok, here it is using std::string.
It is a shorter patch.

Bootstrapped and tested on aarch64.

Ok?

Thanks,
Kyrill

2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/aarch64.c (aarch64_last_printed_arch_string):
     New variable.
     (aarch64_last_printed_tune_string): Likewise.
     (aarch64_declare_function_name): Only output .arch assembler
     directive if it will be different from the previously output
     directive.  Same for .tune comment but only if -dA is set.
     (aarch64_start_file): New function.
     (TARGET_ASM_FILE_START): Define.

2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
     .arch armv8-a\n.  Add -dA to dg-options.
     * gcc.target/aarch64/assembler_arch_1.c: New test.
     * gcc.target/aarch64/target_attr_7.c: Add -dA to dg-options.

> Thanks,
> James
>


[-- Attachment #2: aarch64-asm-cpp.patch --]
[-- Type: text/x-patch, Size: 5839 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 59cbf81474ccdf34e59a4719ee88251bc658ef04..9055229cb31d1b6edad64efb96856610c1277161 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11198,6 +11198,10 @@ aarch64_asm_preferred_eh_data_format (int code ATTRIBUTE_UNUSED, int global)
    return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
 }
 
+/* The last .arch and .tune assembly strings that we printed.  */
+static std::string aarch64_last_printed_arch_string;
+static std::string aarch64_last_printed_tune_string;
+
 /* Implement ASM_DECLARE_FUNCTION_NAME.  Output the ISA features used
    by the function fndecl.  */
 
@@ -11220,23 +11224,55 @@ aarch64_declare_function_name (FILE *stream, const char* name,
   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 ());
+  /* Only update the assembler .arch string if it is distinct from the last
+     such string we printed.  */
+  std::string to_print = this_arch->name + extension;
+  if (to_print != aarch64_last_printed_arch_string)
+    {
+      asm_fprintf (asm_out_file, "\t.arch %s\n", to_print.c_str ());
+      aarch64_last_printed_arch_string = to_print;
+    }
 
   /* Print the cpu name we're tuning for in the comments, might be
-     useful to readers of the generated asm.  */
-
+     useful to readers of the generated asm.  Do it only when it changes
+     from function to function and verbose assembly is requested.  */
   const struct processor *this_tune
     = aarch64_get_tune_cpu (targ_options->x_explicit_tune_core);
 
-  asm_fprintf (asm_out_file, "\t" ASM_COMMENT_START ".tune %s\n",
-	       this_tune->name);
+  if (flag_debug_asm && aarch64_last_printed_tune_string != this_tune->name)
+    {
+      asm_fprintf (asm_out_file, "\t" ASM_COMMENT_START ".tune %s\n",
+		   this_tune->name);
+      aarch64_last_printed_tune_string = this_tune->name;
+    }
 
   /* Don't forget the type directive for ELF.  */
   ASM_OUTPUT_TYPE_DIRECTIVE (stream, name, "function");
   ASM_OUTPUT_LABEL (stream, name);
 }
 
+/* Implements TARGET_ASM_FILE_START.  Output the assembly header.  */
+
+static void
+aarch64_start_file (void)
+{
+  struct cl_target_option *default_options
+    = TREE_TARGET_OPTION (target_option_default_node);
+
+  const struct processor *default_arch
+    = aarch64_get_arch (default_options->x_explicit_arch);
+  unsigned long default_isa_flags = default_options->x_aarch64_isa_flags;
+  std::string extension
+    = aarch64_get_extension_string_for_isa_flags (default_isa_flags);
+
+   aarch64_last_printed_arch_string = default_arch->name + extension;
+   aarch64_last_printed_tune_string = "";
+   asm_fprintf (asm_out_file, "\t.arch %s\n",
+		aarch64_last_printed_arch_string.c_str ());
+
+   default_file_start ();
+}
+
 /* Emit load exclusive.  */
 
 static void
@@ -13970,6 +14006,9 @@ aarch64_optab_supported_p (int op, machine_mode, machine_mode,
 #define TARGET_ASM_CAN_OUTPUT_MI_THUNK \
   hook_bool_const_tree_hwi_hwi_const_tree_true
 
+#undef TARGET_ASM_FILE_START
+#define TARGET_ASM_FILE_START aarch64_start_file
+
 #undef TARGET_ASM_OUTPUT_MI_THUNK
 #define TARGET_ASM_OUTPUT_MI_THUNK aarch64_output_mi_thunk
 
diff --git a/gcc/testsuite/gcc.target/aarch64/assembler_arch_1.c b/gcc/testsuite/gcc.target/aarch64/assembler_arch_1.c
new file mode 100644
index 0000000000000000000000000000000000000000..901e50a178d7a4a443a5ad0abe63f624688db268
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/assembler_arch_1.c
@@ -0,0 +1,20 @@
+/* { dg-do assemble } */
+/* { dg-options "-march=armv8-a" } */
+
+/* Make sure that the function header in assembly doesn't override
+   user asm arch_extension directives.  */
+
+__asm__ (".arch_extension lse");
+
+void
+foo (int i, int *v)
+{
+  register int w0 asm ("w0") = i;
+  register int *x1 asm ("x1") = v;
+
+  asm volatile (
+  "\tstset   %w[i], %[v]\n"
+  : [i] "+r" (w0), [v] "+Q" (v)
+  : "r" (x1)
+  : "x30");
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_1.c b/gcc/testsuite/gcc.target/aarch64/target_attr_1.c
index 852ce1e9f06fe6e9d95f88a036d9012c4aed1c10..0527d0c3d613ca696f63161e54d46cc0060b30fb 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_1.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -mcpu=thunderx -save-temps" } */
+/* { dg-options "-O2 -mcpu=thunderx -dA" } */
 
 /* Test that cpu attribute overrides the command-line -mcpu.  */
 
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_15.c b/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
index 02091c6c542b98b61156409b437d142169348138..f72bec878bf635429d67d441f0ec168839ff9888 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
@@ -10,6 +10,4 @@ foo (int a)
   return a + 1;
 }
 
-/* { dg-final { scan-assembler-not "\\+fp" } } */
-/* { dg-final { scan-assembler-not "\\+crypto" } } */
-/* { dg-final { scan-assembler-not "\\+simd" } } */
+/* { dg-final { scan-assembler-times "\\.arch armv8-a\n" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_7.c b/gcc/testsuite/gcc.target/aarch64/target_attr_7.c
index 32a840378ab4cad8cc90b896be112c7c46bc01a8..818d327705f3d5ec7863da93c4181cc1441f58f5 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_7.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_7.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -mcpu=thunderx -save-temps" } */
+/* { dg-options "-O2 -mcpu=thunderx -dA" } */
 
 /* Make sure that #pragma overrides command line option and
    target attribute overrides the pragma.  */

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[James Greenhalgh]

On Thu, Feb 11, 2016 at 01:10:33PM +0000, Kyrill Tkachov wrote:
> >>>Why not just keep the last string you printed, and use a string compare
> >>>to decide whether to print or not? Sure we'll end up doing a bit more
> >>>work, but the logic becomes simpler to follow and we don't need to pass
> >>>around another struct...
> >>I did do it this way to avoid a string comparison (I try to avoid
> >>manual string manipulations where I can as they're so easy to get wrong)
> >>though this isn't on any hot path.
> >>We don't really pass the structure around anywhere, we just keep one
> >>instance. We'd have to do the same with a string i.e. keep a string
> >>object around that we'd strcpy (or C++ equivalent) a string to every time
> >>we wanted to update it, so I thought this approach is cleaner as the
> >>architecture features are already fully described by a pointer to
> >>an element in the static constant all_architectures table and an
> >>unsigned long holding the ISA flags.
> >>
> >>If you insist I can change it to a string, but I personally don't
> >>think it's worth it.
> >Had you been working on a C string I probably wouldn't have noticed. But
> >you're already working with C++ strings in this function, so much of what
> >you are concerned about is straightforward.
> >
> >I'd encourage you to try it using idiomatic string manipulation in C++, the
> >cleanup should be worth it.
> 
> Ok, here it is using std::string.
> It is a shorter patch.

Looks good to me, OK for trunk.

Thanks,
James

> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * config/aarch64/aarch64.c (aarch64_last_printed_arch_string):
>     New variable.
>     (aarch64_last_printed_tune_string): Likewise.
>     (aarch64_declare_function_name): Only output .arch assembler
>     directive if it will be different from the previously output
>     directive.  Same for .tune comment but only if -dA is set.
>     (aarch64_start_file): New function.
>     (TARGET_ASM_FILE_START): Define.
> 
> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>     .arch armv8-a\n.  Add -dA to dg-options.
>     * gcc.target/aarch64/assembler_arch_1.c: New test.
>     * gcc.target/aarch64/target_attr_7.c: Add -dA to dg-options.
> 

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Christophe Lyon]

On 11 February 2016 at 14:10, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> On 10/02/16 10:39, James Greenhalgh wrote:
>>
>> On Wed, Feb 10, 2016 at 10:32:16AM +0000, Kyrill Tkachov wrote:
>>>
>>> Hi James,
>>>
>>> On 10/02/16 10:11, James Greenhalgh wrote:
>>>>
>>>> On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> As part of the target attributes and pragmas support for GCC 6 I
>>>>> changed the
>>>>> aarch64 port to emit a .arch assembly directive for each function that
>>>>> describes the architectural features used by that function.  This is a
>>>>> change
>>>>
>>>> >from GCC 5 behaviour where we output a single .arch directive at the
>>>>>
>>>>> beginning of the assembly file corresponding to architectural features
>>>>> given
>>>>> on the command line.
>>>>
>>>> <snip>
>>>>>
>>>>> Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I
>>>>> managed
>>>>> to build a recent allyesconfig Linux kernel where before the build
>>>>> would fail
>>>>> when assembling the LSE instructions.
>>>>>
>>>>> Ok for trunk?
>>>>
>>>> One comment, that I'm willing to be convinced on...
>>>>
>>>>> Thanks,
>>>>> Kyrill
>>>>>
>>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>
>>>>>      * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>>>>>      New struct definition.
>>>>>      (aarch64_previous_asm_output): New variable.
>>>>>      (aarch64_declare_function_name): Only output .arch assembler
>>>>>      directive if it will be different from the previously output
>>>>>      directive.
>>>>>      (aarch64_start_file): New function.
>>>>>      (TARGET_ASM_FILE_START): Define.
>>>>>
>>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>
>>>>>      * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>>>>>      Delete unneeded -save-temps.
>>>>>      * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>>>>>      * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>>>>      .arch armv8-a\n.
>>>>>      * gcc.target/aarch64/assembler_arch_1.c: New test.
>>>>> commit 2df0f24332e316b8d18d4571438f76726a0326e7
>>>>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>>>> Date:   Wed Jan 27 12:54:54 2016 +0000
>>>>>
>>>>>      [AArch64] Only update assembler .arch directive when necessary
>>>>>
>>>>> diff --git a/gcc/config/aarch64/aarch64.c
>>>>> b/gcc/config/aarch64/aarch64.c
>>>>> index 5ca2ae8..0751440 100644
>>>>> --- a/gcc/config/aarch64/aarch64.c
>>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>>> @@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code
>>>>> ATTRIBUTE_UNUSED, int global)
>>>>>      return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
>>>>>   }
>>>>> +struct aarch64_output_asm_info
>>>>> +{
>>>>> +  const struct processor *arch;
>>>>> +  const struct processor *cpu;
>>>>> +  unsigned long isa_flags;
>>>>
>>>> Why not just keep the last string you printed, and use a string compare
>>>> to decide whether to print or not? Sure we'll end up doing a bit more
>>>> work, but the logic becomes simpler to follow and we don't need to pass
>>>> around another struct...
>>>
>>> I did do it this way to avoid a string comparison (I try to avoid
>>> manual string manipulations where I can as they're so easy to get wrong)
>>> though this isn't on any hot path.
>>> We don't really pass the structure around anywhere, we just keep one
>>> instance. We'd have to do the same with a string i.e. keep a string
>>> object around that we'd strcpy (or C++ equivalent) a string to every time
>>> we wanted to update it, so I thought this approach is cleaner as the
>>> architecture features are already fully described by a pointer to
>>> an element in the static constant all_architectures table and an
>>> unsigned long holding the ISA flags.
>>>
>>> If you insist I can change it to a string, but I personally don't
>>> think it's worth it.
>>
>> Had you been working on a C string I probably wouldn't have noticed. But
>> you're already working with C++ strings in this function, so much of what
>> you are concerned about is straightforward.
>>
>> I'd encourage you to try it using idiomatic string manipulation in C++,
>> the
>> cleanup should be worth it.
>
>
> Ok, here it is using std::string.
> It is a shorter patch.
>
> Bootstrapped and tested on aarch64.
>
> Ok?
>
> Thanks,
> Kyrill
>
> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * config/aarch64/aarch64.c (aarch64_last_printed_arch_string):
>     New variable.
>     (aarch64_last_printed_tune_string): Likewise.
>     (aarch64_declare_function_name): Only output .arch assembler
>     directive if it will be different from the previously output
>     directive.  Same for .tune comment but only if -dA is set.
>     (aarch64_start_file): New function.
>     (TARGET_ASM_FILE_START): Define.
>
> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>     .arch armv8-a\n.  Add -dA to dg-options.
>     * gcc.target/aarch64/assembler_arch_1.c: New test.

Hi Kyrill,

I'm seeing this new test fail, presumably because the binutils I use
are too old:
/cckXrDIS.s: Assembler messages:
/cckXrDIS.s:4: Error: unknown pseudo-op: `.arch_extension'
/cckXrDIS.s:20: Error: selected processor does not support `stset w0,[x2]'

Do we want to guard the test against such cases and query binutils
capabilities?


>     * gcc.target/aarch64/target_attr_7.c: Add -dA to dg-options.
>
>> Thanks,
>> James
>>
>

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Kyrill Tkachov]

On 11/02/16 17:57, Christophe Lyon wrote:
> On 11 February 2016 at 14:10, Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
>> On 10/02/16 10:39, James Greenhalgh wrote:
>>> On Wed, Feb 10, 2016 at 10:32:16AM +0000, Kyrill Tkachov wrote:
>>>> Hi James,
>>>>
>>>> On 10/02/16 10:11, James Greenhalgh wrote:
>>>>> On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> As part of the target attributes and pragmas support for GCC 6 I
>>>>>> changed the
>>>>>> aarch64 port to emit a .arch assembly directive for each function that
>>>>>> describes the architectural features used by that function.  This is a
>>>>>> change
>>>>> >from GCC 5 behaviour where we output a single .arch directive at the
>>>>>> beginning of the assembly file corresponding to architectural features
>>>>>> given
>>>>>> on the command line.
>>>>> <snip>
>>>>>> Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I
>>>>>> managed
>>>>>> to build a recent allyesconfig Linux kernel where before the build
>>>>>> would fail
>>>>>> when assembling the LSE instructions.
>>>>>>
>>>>>> Ok for trunk?
>>>>> One comment, that I'm willing to be convinced on...
>>>>>
>>>>>> Thanks,
>>>>>> Kyrill
>>>>>>
>>>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>>
>>>>>>       * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>>>>>>       New struct definition.
>>>>>>       (aarch64_previous_asm_output): New variable.
>>>>>>       (aarch64_declare_function_name): Only output .arch assembler
>>>>>>       directive if it will be different from the previously output
>>>>>>       directive.
>>>>>>       (aarch64_start_file): New function.
>>>>>>       (TARGET_ASM_FILE_START): Define.
>>>>>>
>>>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>>
>>>>>>       * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>>>>>>       Delete unneeded -save-temps.
>>>>>>       * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>>>>>>       * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>>>>>       .arch armv8-a\n.
>>>>>>       * gcc.target/aarch64/assembler_arch_1.c: New test.
>>>>>> commit 2df0f24332e316b8d18d4571438f76726a0326e7
>>>>>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>>>>> Date:   Wed Jan 27 12:54:54 2016 +0000
>>>>>>
>>>>>>       [AArch64] Only update assembler .arch directive when necessary
>>>>>>
>>>>>> diff --git a/gcc/config/aarch64/aarch64.c
>>>>>> b/gcc/config/aarch64/aarch64.c
>>>>>> index 5ca2ae8..0751440 100644
>>>>>> --- a/gcc/config/aarch64/aarch64.c
>>>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>>>> @@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int code
>>>>>> ATTRIBUTE_UNUSED, int global)
>>>>>>       return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel | type;
>>>>>>    }
>>>>>> +struct aarch64_output_asm_info
>>>>>> +{
>>>>>> +  const struct processor *arch;
>>>>>> +  const struct processor *cpu;
>>>>>> +  unsigned long isa_flags;
>>>>> Why not just keep the last string you printed, and use a string compare
>>>>> to decide whether to print or not? Sure we'll end up doing a bit more
>>>>> work, but the logic becomes simpler to follow and we don't need to pass
>>>>> around another struct...
>>>> I did do it this way to avoid a string comparison (I try to avoid
>>>> manual string manipulations where I can as they're so easy to get wrong)
>>>> though this isn't on any hot path.
>>>> We don't really pass the structure around anywhere, we just keep one
>>>> instance. We'd have to do the same with a string i.e. keep a string
>>>> object around that we'd strcpy (or C++ equivalent) a string to every time
>>>> we wanted to update it, so I thought this approach is cleaner as the
>>>> architecture features are already fully described by a pointer to
>>>> an element in the static constant all_architectures table and an
>>>> unsigned long holding the ISA flags.
>>>>
>>>> If you insist I can change it to a string, but I personally don't
>>>> think it's worth it.
>>> Had you been working on a C string I probably wouldn't have noticed. But
>>> you're already working with C++ strings in this function, so much of what
>>> you are concerned about is straightforward.
>>>
>>> I'd encourage you to try it using idiomatic string manipulation in C++,
>>> the
>>> cleanup should be worth it.
>>
>> Ok, here it is using std::string.
>> It is a shorter patch.
>>
>> Bootstrapped and tested on aarch64.
>>
>> Ok?
>>
>> Thanks,
>> Kyrill
>>
>> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * config/aarch64/aarch64.c (aarch64_last_printed_arch_string):
>>      New variable.
>>      (aarch64_last_printed_tune_string): Likewise.
>>      (aarch64_declare_function_name): Only output .arch assembler
>>      directive if it will be different from the previously output
>>      directive.  Same for .tune comment but only if -dA is set.
>>      (aarch64_start_file): New function.
>>      (TARGET_ASM_FILE_START): Define.
>>
>> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>      .arch armv8-a\n.  Add -dA to dg-options.
>>      * gcc.target/aarch64/assembler_arch_1.c: New test.
> Hi Kyrill,

Hi Christophe,

> I'm seeing this new test fail, presumably because the binutils I use
> are too old:
> /cckXrDIS.s: Assembler messages:
> /cckXrDIS.s:4: Error: unknown pseudo-op: `.arch_extension'
> /cckXrDIS.s:20: Error: selected processor does not support `stset w0,[x2]'
>
> Do we want to guard the test against such cases and query binutils
> capabilities?

Hmmm, I'd guess it depends on the effort.
I suppose it would involve creating an effective target check
that tries to assemble a .arch_extension and then also the stset instruction?
Do we have precedent for checking assembler capabilities?

Kyrill

>
>>      * gcc.target/aarch64/target_attr_7.c: Add -dA to dg-options.
>>
>>> Thanks,
>>> James
>>>

Re: [PATCH][AArch64] Only update assembler .arch directive when necessary

[Christophe Lyon]

On 11 February 2016 at 19:04, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> On 11/02/16 17:57, Christophe Lyon wrote:
>>
>> On 11 February 2016 at 14:10, Kyrill Tkachov
>> <kyrylo.tkachov@foss.arm.com> wrote:
>>>
>>> On 10/02/16 10:39, James Greenhalgh wrote:
>>>>
>>>> On Wed, Feb 10, 2016 at 10:32:16AM +0000, Kyrill Tkachov wrote:
>>>>>
>>>>> Hi James,
>>>>>
>>>>> On 10/02/16 10:11, James Greenhalgh wrote:
>>>>>>
>>>>>> On Thu, Feb 04, 2016 at 01:50:31PM +0000, Kyrill Tkachov wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> As part of the target attributes and pragmas support for GCC 6 I
>>>>>>> changed the
>>>>>>> aarch64 port to emit a .arch assembly directive for each function
>>>>>>> that
>>>>>>> describes the architectural features used by that function.  This is
>>>>>>> a
>>>>>>> change
>>>>>>
>>>>>> >from GCC 5 behaviour where we output a single .arch directive at the
>>>>>>>
>>>>>>> beginning of the assembly file corresponding to architectural
>>>>>>> features
>>>>>>> given
>>>>>>> on the command line.
>>>>>>
>>>>>> <snip>
>>>>>>>
>>>>>>> Bootstrapped and tested on aarch64-none-linux-gnu.  With this patch I
>>>>>>> managed
>>>>>>> to build a recent allyesconfig Linux kernel where before the build
>>>>>>> would fail
>>>>>>> when assembling the LSE instructions.
>>>>>>>
>>>>>>> Ok for trunk?
>>>>>>
>>>>>> One comment, that I'm willing to be convinced on...
>>>>>>
>>>>>>> Thanks,
>>>>>>> Kyrill
>>>>>>>
>>>>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>>>
>>>>>>>       * config/aarch64/aarch64.c (struct aarch64_output_asm_info):
>>>>>>>       New struct definition.
>>>>>>>       (aarch64_previous_asm_output): New variable.
>>>>>>>       (aarch64_declare_function_name): Only output .arch assembler
>>>>>>>       directive if it will be different from the previously output
>>>>>>>       directive.
>>>>>>>       (aarch64_start_file): New function.
>>>>>>>       (TARGET_ASM_FILE_START): Define.
>>>>>>>
>>>>>>> 2016-02-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>>>
>>>>>>>       * gcc.target/aarch64/assembler_arch_1.c: Add -dA to dg-options.
>>>>>>>       Delete unneeded -save-temps.
>>>>>>>       * gcc.target/aarch64/assembler_arch_7.c: Likewise.
>>>>>>>       * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>>>>>>       .arch armv8-a\n.
>>>>>>>       * gcc.target/aarch64/assembler_arch_1.c: New test.
>>>>>>> commit 2df0f24332e316b8d18d4571438f76726a0326e7
>>>>>>> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>>>>>> Date:   Wed Jan 27 12:54:54 2016 +0000
>>>>>>>
>>>>>>>       [AArch64] Only update assembler .arch directive when necessary
>>>>>>>
>>>>>>> diff --git a/gcc/config/aarch64/aarch64.c
>>>>>>> b/gcc/config/aarch64/aarch64.c
>>>>>>> index 5ca2ae8..0751440 100644
>>>>>>> --- a/gcc/config/aarch64/aarch64.c
>>>>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>>>>> @@ -11163,6 +11163,17 @@ aarch64_asm_preferred_eh_data_format (int
>>>>>>> code
>>>>>>> ATTRIBUTE_UNUSED, int global)
>>>>>>>       return (global ? DW_EH_PE_indirect : 0) | DW_EH_PE_pcrel |
>>>>>>> type;
>>>>>>>    }
>>>>>>> +struct aarch64_output_asm_info
>>>>>>> +{
>>>>>>> +  const struct processor *arch;
>>>>>>> +  const struct processor *cpu;
>>>>>>> +  unsigned long isa_flags;
>>>>>>
>>>>>> Why not just keep the last string you printed, and use a string
>>>>>> compare
>>>>>> to decide whether to print or not? Sure we'll end up doing a bit more
>>>>>> work, but the logic becomes simpler to follow and we don't need to
>>>>>> pass
>>>>>> around another struct...
>>>>>
>>>>> I did do it this way to avoid a string comparison (I try to avoid
>>>>> manual string manipulations where I can as they're so easy to get
>>>>> wrong)
>>>>> though this isn't on any hot path.
>>>>> We don't really pass the structure around anywhere, we just keep one
>>>>> instance. We'd have to do the same with a string i.e. keep a string
>>>>> object around that we'd strcpy (or C++ equivalent) a string to every
>>>>> time
>>>>> we wanted to update it, so I thought this approach is cleaner as the
>>>>> architecture features are already fully described by a pointer to
>>>>> an element in the static constant all_architectures table and an
>>>>> unsigned long holding the ISA flags.
>>>>>
>>>>> If you insist I can change it to a string, but I personally don't
>>>>> think it's worth it.
>>>>
>>>> Had you been working on a C string I probably wouldn't have noticed. But
>>>> you're already working with C++ strings in this function, so much of
>>>> what
>>>> you are concerned about is straightforward.
>>>>
>>>> I'd encourage you to try it using idiomatic string manipulation in C++,
>>>> the
>>>> cleanup should be worth it.
>>>
>>>
>>> Ok, here it is using std::string.
>>> It is a shorter patch.
>>>
>>> Bootstrapped and tested on aarch64.
>>>
>>> Ok?
>>>
>>> Thanks,
>>> Kyrill
>>>
>>> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      * config/aarch64/aarch64.c (aarch64_last_printed_arch_string):
>>>      New variable.
>>>      (aarch64_last_printed_tune_string): Likewise.
>>>      (aarch64_declare_function_name): Only output .arch assembler
>>>      directive if it will be different from the previously output
>>>      directive.  Same for .tune comment but only if -dA is set.
>>>      (aarch64_start_file): New function.
>>>      (TARGET_ASM_FILE_START): Define.
>>>
>>> 2016-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      * gcc.target/aarch64/target_attr_15.c: Scan assembly for
>>>      .arch armv8-a\n.  Add -dA to dg-options.
>>>      * gcc.target/aarch64/assembler_arch_1.c: New test.
>>
>> Hi Kyrill,
>
>
> Hi Christophe,
>
>> I'm seeing this new test fail, presumably because the binutils I use
>> are too old:
>> /cckXrDIS.s: Assembler messages:
>> /cckXrDIS.s:4: Error: unknown pseudo-op: `.arch_extension'
>> /cckXrDIS.s:20: Error: selected processor does not support `stset w0,[x2]'
>>
>> Do we want to guard the test against such cases and query binutils
>> capabilities?
>
>
> Hmmm, I'd guess it depends on the effort.
> I suppose it would involve creating an effective target check
> that tries to assemble a .arch_extension and then also the stset
> instruction?
> Do we have precedent for checking assembler capabilities?
>
I thought ARM added something similar in 2015, but maybe it
was in gcc's configure instead of in the testsuite (maybe related
to 8.1).
I can't remember atm.


> Kyrill
>
>
>>
>>>      * gcc.target/aarch64/target_attr_7.c: Add -dA to dg-options.
>>>
>>>> Thanks,
>>>> James
>>>>
>