From patchwork Thu Feb 11 13:10:33 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 581881 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 93E4F140BA5 for ; Fri, 12 Feb 2016 00:10:48 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=MR/3leK+; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=BY8gKsIStY443cQl7 WhPDaMYR69o1W0CqWTkUAmPXdd8uWCtLiYQoa0J6SBIsOe9DFqMlDMnueSTFpZXx Y7tGYE6CxUdCh6luel7cCDHP7Fj9c3UWJ94/7xgGO7ITctJQsY2xaqn8xhbJPdmZ W4tqGEV2hLeZdC3xq9NM0c4NPo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=GBD5ZgeydxA7HlDXgciYBFM 1W8E=; b=MR/3leK+o79VDp0wF7q7ViYEkjmrWhNXrI8E198RqtAX91zY+TmKspd cnMzFqho66xgmDOwwBlAlrepeaauDsxfJI7d0VkaVqaAz4qhnBgDNav8P2KeNLZZ tYZql64Hc6PlmDRFeivQpfzJ9Z4eEQSaBgvYn9edstn2HFWtNEh8= Received: (qmail 14626 invoked by alias); 11 Feb 2016 13:10:40 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 14604 invoked by uid 89); 11 Feb 2016 13:10:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, KAM_LOTSOFHASH, RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=10, 6, 1, 20, Implements, readers X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 11 Feb 2016 13:10:37 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E115549; Thu, 11 Feb 2016 05:09:47 -0800 (PST) Received: from [10.2.206.200] (e100706-lin.cambridge.arm.com [10.2.206.200]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 963A53F238; Thu, 11 Feb 2016 05:10:34 -0800 (PST) Message-ID: <56BC8849.90704@foss.arm.com> Date: Thu, 11 Feb 2016 13:10:33 +0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: James Greenhalgh CC: GCC Patches , Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH][AArch64] Only update assembler .arch directive when necessary References: <56B35727.6030104@foss.arm.com> <20160210101101.GA22860@arm.com> <56BB11B0.2080209@foss.arm.com> <20160210103904.GA23634@arm.com> In-Reply-To: <20160210103904.GA23634@arm.com> 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. >>> >>>> 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 >>>> >>>> * 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 >>>> >>>> * 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 >>>> 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 * 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 * 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 > 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. */