On 20/07/15 17:44, James Greenhalgh wrote:
> 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.

Yes, this is redundant.

>
>> 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 for the review, here's an updated version.
In the end, I chose to retain the use alloca (other patches in the series
are reworked to use it too).

How's this?

Thanks,
Kyrill

2015-07-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

      * config/aarch64/aarch64.opt (aarch64_arch_string): Delete.
      (aarch64_cpu_string): Likewise.
      (aarch64_tune_string): Likewise.
      * config/aarch64/aarch64.c (aarch64_parse_opt_result): New enum.
      (aarch64_parse_extension): Return aarch64_parse_opt_result.
      Add extra argument to put result into.
      (aarch64_parse_arch): Likewise.  Do not set selected_cpu.
      (aarch64_parse_cpu): Add arguments to put results into. Return
      aarch64_parse_opt_result.
      (aarch64_parse_tune): Likewise.
      (aarch64_override_options_after_change_1): New function.
      (aarch64_override_options_internal): New function.
      (aarch64_validate_mcpu): Likewise.
      (aarch64_validate_march): Likewise.
      (aarch64_validate_mtune): Likewise.
      (aarch64_override_options): Update to reflect above changes.
      Move some logic into aarch64_override_options_internal.
      Initialize target_option_default_node and target_option_current_node.
      (aarch64_override_options_after_change): Move logic into
      aarch64_override_options_after_change_1 and call it with global_options.
      (initialize_aarch64_code_model): Take a gcc_options pointer and use the
      flag values from that.

>
> Thanks,
> James
>