From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11090 invoked by alias); 21 Jul 2015 11:15:28 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 11076 invoked by uid 89); 21 Jul 2015 11:15:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 21 Jul 2015 11:15:26 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-13-GL_jqRATSuGkJxT1t_lnlg-1; Tue, 21 Jul 2015 12:15:21 +0100 Received: from [10.2.207.50] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Tue, 21 Jul 2015 12:15:21 +0100 Message-ID: <55AE29C9.50606@arm.com> Date: Tue, 21 Jul 2015 11:44:00 -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][6/14] Implement TARGET_OPTION_SAVE/TARGET_OPTION_RESTORE References: <55A7CBD2.7000302@arm.com> <55A90EE8.5040104@arm.com> <20150721110904.GA10028@arm.com> In-Reply-To: <20150721110904.GA10028@arm.com> X-MC-Unique: GL_jqRATSuGkJxT1t_lnlg-1 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg01739.txt.bz2 On 21/07/15 12:09, James Greenhalgh wrote: > On Fri, Jul 17, 2015 at 03:19:20PM +0100, Kyrill Tkachov wrote: >> >> This is a slight respin of this patch, handling the -moverride string mo= re gracefully. >> We need to explicitly save and restore it in TARGET_OPTION_SAVE otherwis= e the option gen machinery >> gets confused about its type and during its printing uses the wrong form= at code for the pointer, leading to a warning that may trigger during boots= trap. >> >> Otherwise it is the same as the previous version. >> >> Bootstrapped and tested on aarch64. >> I'd like to propose this version instead of the original. >> >> Ok? > The ChangeLog looks outdated with respect to the patch, in particular: > >> (x_aarch64_isa_flags): Likewise. > This is just "aarch64_isa_flags" in the file. > >> +TargetSave >> +const char *x_aarch64_override_tune_string >> + > This is not mentioned. > >> (master=3D): Likewise. > This doesn't exist. > >> +#undef TARGET_OPTION_SAVE >> +#define TARGET_OPTION_SAVE aarch64_option_save >> + > This is not mentioned. > > In addition to the ChangeLog nits, Oops, thanks for catching those, here's an updated one. 2015-07-21 Kyrylo Tkachov * config/aarch64/aarch64.opt (explicit_tune_core): New TargetVariable. (explicit_arch): Likewise. (aarch64_isa_flags): Likewise. (x_aarch64_override_tune_string): Likewise. (mgeneral-regs-only): Mark as Save. (mfix-cortex-a53-835769): Likewise. (mcmodel=3D): Likewise. (mstrict-align): Likewise. (momit-leaf-frame-pointer): Likewise. (mtls-dialect): Likewise. * config/aarch64/aarch64.h (ASM_DECLARE_FUNCTION_NAME): Define. (aarch64_isa_flags): Remove extern declaration. * config/aarch64/aarch64.c (aarch64_validate_mcpu): Return a bool to indicate success or failure. (aarch64_validate_march): Likewise. (aarch64_validate_mtune): Likewise. (aarch64_isa_flags): Delete. (aarch64_override_options_internal): Access opts->x_aarch64_isa_flags instead of aarch64_isa_flags. (aarch64_get_tune_cpu): New function. (aarch64_get_arch): Likewise. (aarch64_override_options): Use above and set up explicit_tune_core and explicit_arch. (aarch64_print_extension): Move earlier in file. Add isa_flags argument and use that instead of the global aarch64_isa_flags. (aarch64_option_restore): Likewise. (aarch64_option_print): Likewise. (aarch64_declare_function_name): Likewise. (aarch64_start_file): Delete. (TARGET_ASM_FILE_START): Do not define. (TARGET_OPTION_SAVE, TARGET_OPTION_RESTORE, TARGET_OPTION_PRINT): Define. * config/aarch64/aarch64-protos.h (aarch64_declare_function_name): Declare prototype. > >> +/* Return the CPU corresponding to the enum CPU. >> + If it doesn't specify a cpu, return the default. */ >> + >> +static const struct processor * >> +aarch64_get_tune_cpu (enum aarch64_processor cpu) >> +{ >> + if (cpu !=3D aarch64_none) >> + return &all_cores[cpu]; >> + >> + return &all_cores[TARGET_CPU_DEFAULT & 0x3f]; >> +} > This looks strange to me, is there nothing we can do to make it clear > what "0x3f" means in this context, or better yet to get rid of the > two-in-one variable... This is due to the cryptic way in which we specify the configure-time cpu and arch extensions. Perhaps a comment at the default definition of TARGET_CPU_DEFAULT in aarch64.h would partially mitigate the confusion. Thanks, Kyrill > > OK with a fixed ChangeLog. > > Thanks, > James >