diff mbox

[AArch64] Only update assembler .arch directive when necessary

Message ID 56BC8849.90704@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov Feb. 11, 2016, 1:10 p.m. UTC
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
>

Comments

James Greenhalgh Feb. 11, 2016, 1:19 p.m. UTC | #1
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.
>
Christophe Lyon Feb. 11, 2016, 5:57 p.m. UTC | #2
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
>>
>
Kyrill Tkachov Feb. 11, 2016, 6:04 p.m. UTC | #3
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
>>>
Christophe Lyon Feb. 11, 2016, 6:16 p.m. UTC | #4
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
>>>>
>
diff mbox

Patch

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.  */