From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29570 invoked by alias); 20 Jul 2015 16:44:32 -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 29558 invoked by uid 89); 20 Jul 2015 16:44:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 20 Jul 2015 16:44:29 +0000 Received: from arm.com (e106375-lin.cambridge.arm.com [10.2.207.23]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id t6KGiQ5G006918; Mon, 20 Jul 2015 17:44:26 +0100 Date: Mon, 20 Jul 2015 16:58:00 -0000 From: James Greenhalgh To: Kyrill Tkachov Cc: GCC Patches , Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH][AArch64][3/14] Refactor option override code Message-ID: <20150720164426.GA20088@arm.com> References: <55A7CBC5.3030201@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55A7CBC5.3030201@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg01677.txt.bz2 On Thu, Jul 16, 2015 at 04:20:37PM +0100, Kyrill Tkachov wrote: > Hi all, > > This one is more meaty than the previous ones. It buffs up the parsing functions for > the mcpu, march, mtune options, decouples them and makes them return an enum describing > the errors that may occur. This will allow us to use these functions in other contexts > beyond aarch64_override_options. > > aarch64_override_options itself gets an overhaul and is split up into code that must run > only once after the command line option have been processed, and code that has to be run > every time the backend-specific state changes (after SWITCHABLE_TARGET is implemented). > > The stuff that must be run every time the backend state changes is put into > aarch64_override_options_internal. > > Also, this patch deletes the declarations of aarch64_{arch,cpu,tune}_string from aarch64.opt > as they are superfluous since the march, mtune and mcpu option specification implicitly > declares these variables. > > This patch looks large, but a lot of it is moving code around... > > Bootstrapped and tested as part of the series on aarch64. > > Ok for trunk? I'm a bit hazy on the logic of one part, that is the refactoring of aarch64_override_options_after_change in to aarch64_override_options_after_change_1. It seems like if we have this hunk: > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 32b974a..5ea65e3 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -7473,85 +7507,253 @@ aarch64_parse_override_string (const char* input_string, > free (string_root); > } > > -/* Implement TARGET_OPTION_OVERRIDE. */ > > static void > -aarch64_override_options (void) > +aarch64_override_options_after_change_1 (struct gcc_options *opts) > { > - /* -mcpu=CPU is shorthand for -march=ARCH_FOR_CPU, -mtune=CPU. > - If either of -march or -mtune is given, they override their > - respective component of -mcpu. > + if (opts->x_flag_omit_frame_pointer) > + opts->x_flag_omit_leaf_frame_pointer = false; > + else if (opts->x_flag_omit_leaf_frame_pointer) > + opts->x_flag_omit_frame_pointer = true; > > - So, first parse AARCH64_CPU_STRING, then the others, be careful > - with -march as, if -mcpu is not present on the command line, march > - must set a sensible default CPU. */ > - if (aarch64_cpu_string) > + /* If not opzimizing for size, set the default > + alignment to what the target wants. */ > + if (!opts->x_optimize_size) > { > - aarch64_parse_cpu (); > + if (opts->x_align_loops <= 0) > + opts->x_align_loops = aarch64_tune_params.loop_align; > + if (opts->x_align_jumps <= 0) > + opts->x_align_jumps = aarch64_tune_params.jump_align; > + if (opts->x_align_functions <= 0) > + opts->x_align_functions = aarch64_tune_params.function_align; > } > +} Then this code left behind in aarch64_override_options_after_change : > > if (flag_omit_frame_pointer) > flag_omit_leaf_frame_pointer = false; > else if (flag_omit_leaf_frame_pointer) > flag_omit_frame_pointer = true; > > /* If not optimizing for size, set the default > alignment to what the target wants */ > if (!optimize_size) > { > if (align_loops <= 0) > align_loops = aarch64_tune_params.loop_align; > if (align_jumps <= 0) > align_jumps = aarch64_tune_params.jump_align; > if (align_functions <= 0) > align_functions = aarch64_tune_params.function_align; > } is redundant/misleading. > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 32b974a..5ea65e3 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -7101,12 +7101,27 @@ aarch64_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind, > return retval; > } > > -static void initialize_aarch64_code_model (void); > +static void initialize_aarch64_code_model (struct gcc_options *); > > -/* Parse the architecture extension string. */ > +/* Enum describing the various ways that the > + aarch64_parse_{arch,tune,cpu,extension} functions can fail. > + This way their callers can choose what kind of error to give. */ > > -static void > -aarch64_parse_extension (char *str) > +enum aarch64_parse_opt_result > +{ > + AARCH64_PARSE_OK, /* Parsing was successful. */ > + AARCH64_PARSE_MISSING_ARG, /* Missing argument. */ > + AARCH64_PARSE_INVALID_FEATURE, /* Invalid feature modifier. */ > + AARCH64_PARSE_INVALID_ARG /* Invalid arch, tune, cpu arg. */ > +}; > + > + Extra newline here. > -/* Parse the ARCH string. */ > +/* Parse the TO_PARSE string and put the architecture struct that it > + selects into RES and the architectural features into ISA_FLAGS. > + Return an aarch64_parse_opt_result describing the parse result. > + If there is an error parsing, RES and ISA_FLAGS are left unchanged. */ > > -static void > -aarch64_parse_arch (void) > +static enum aarch64_parse_opt_result > +aarch64_parse_arch (const char *to_parse, const struct processor **res, > + unsigned long *isa_flags) > { > char *ext; > const struct processor *arch; > - char *str = (char *) alloca (strlen (aarch64_arch_string) + 1); > + char *str = xstrdup (to_parse); > size_t len; > > - strcpy (str, aarch64_arch_string); > + strcpy (str, to_parse); Is this really needed after the xstrdup you used above? > -/* Parse the CPU string. */ > +/* Parse the TO_PARSE string and put the result tuning in RES and the > + architecture flags in ISA_FLAGS. Return an aarch64_parse_opt_result > + describing the parse result. If there is an error parsing, RES and > + ISA_FLAGS are left unchanged. */ > > -static void > -aarch64_parse_cpu (void) > +static enum aarch64_parse_opt_result > +aarch64_parse_cpu (const char *to_parse, const struct processor **res, > + unsigned long *isa_flags) > { > char *ext; > const struct processor *cpu; > - char *str = (char *) alloca (strlen (aarch64_cpu_string) + 1); > + char *str = xstrdup (to_parse); > size_t len; > > - strcpy (str, aarch64_cpu_string); > + strcpy (str, to_parse); As above, is this needed after the xstrdup? > -/* Parse the TUNE string. */ > +/* Parse the TO_PARSE string and put the cpu it selects into RES. > + Return an aarch64_parse_opt_result describing the parse result. > + If the parsing fails the RES does not change. */ > > -static void > -aarch64_parse_tune (void) > +static enum aarch64_parse_opt_result > +aarch64_parse_tune (const char *to_parse, const struct processor **res) > { > const struct processor *cpu; > - char *str = (char *) alloca (strlen (aarch64_tune_string) + 1); > - strcpy (str, aarch64_tune_string); > + char *str = xstrdup (to_parse); > + strcpy (str, to_parse); Likewise. > +/* 'Unpack' up the internal tuning structs and update the options > + in OPTS. The caller must have set up selected_tune and selected_arch > + as all the other target-specific codegen decisions are > + derived from them. */ > + > +static void > +aarch64_override_options_internal (struct gcc_options *opts) > +{ > + aarch64_tune_flags = selected_tune->flags; > + aarch64_tune = selected_tune->sched_core; > + /* Make a copy of the tuning parameters attached to the core, which > + we may later overwrite. */ > + aarch64_tune_params = *(selected_tune->tune); > + aarch64_architecture_version = selected_arch->architecture_version; > + > + if (opts->x_aarch64_override_tune_string) > + aarch64_parse_override_string (opts->x_aarch64_override_tune_string, > + &aarch64_tune_params); > + > + /* This target defaults to strict volatile bitfields. */ > + if (opts->x_flag_strict_volatile_bitfields < 0 && abi_version_at_least (2)) > + opts->x_flag_strict_volatile_bitfields = 1; > + > + if (opts->x_aarch64_fix_a53_err835769 == 2) > { > - aarch64_parse_arch (); > +#ifdef TARGET_FIX_ERR_A53_835769_DEFAULT > + opts->x_aarch64_fix_a53_err835769 = 1; > +#else > + opts->x_aarch64_fix_a53_err835769 = 0; > +#endif > } > > - if (aarch64_tune_string) > + /* -mgeneral-regs-only sets a mask in target_flags, make sure that > + aarch64_isa_flags does not contain the FP/SIMD/Crypto feature flags > + in case some code tries reading aarch64_isa_flags directly to check if > + FP is available. Reuse the aarch64_parse_extension machinery since it > + knows how to disable any other flags that fp implies. */ > + if (TARGET_GENERAL_REGS_ONLY_P (opts->x_target_flags)) > { > - aarch64_parse_tune (); > + /* aarch64_parse_extension takes char* rather than const char* because > + it is usually called from within other parsing functions. */ > + char tmp_str[6]; > + strcpy (tmp_str, "+nofp"); Just: char tmp_str[] = "+nofp"; ? Thanks, James