Re: [PATCH][AArch64][6/14] Implement TARGET_OPTION_SAVE/TARGET_OPTION_RESTORE

[Kyrill Tkachov <kyrylo.tkachov@arm.com>]

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 more gracefully.
>> We need to explicitly save and restore it in TARGET_OPTION_SAVE otherwise the option gen machinery
>> gets confused about its type and during its printing uses the wrong format code for the pointer, leading to a warning that may trigger during bootstrap.
>>
>> 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=): 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  <kyrylo.tkachov@arm.com>

     * 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=): 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 != 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
>