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

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

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