On 11 February 2016 at 19:04, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com> wrote: > > On 11/02/16 17:57, Christophe Lyon wrote: >> >> On 11 February 2016 at 14:10, Kyrill Tkachov >> <kyrylo.tkac...@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.tkac...@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.tkac...@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.tkac...@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.tkac...@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.tkac...@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 >>>> >