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

[Christophe Lyon <christophe dot lyon at linaro dot org>]

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