This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]