[PATCH][AArch64][3/14] Refactor option override code

[James Greenhalgh]

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