On 10/4/18 11:29 AM, Kyrill Tkachov wrote:
> Hi Martin,
>
> On 18/07/18 16:49, Martin Liška wrote:
>> Hi.
>>
>> This patch improves aarch64 feature modifier hints.
>>
>> May I please ask ARM folks to test the patch?
>> Thanks,
>> Martin
>>
>
> I've bootstrapped and tested it on aarch64-none-linux-gnu and I didn't see
> any problems.
> I like the functionality! It is definitely an improvement to the user
> experience.
>
> I'm not an aarch64 maintainer but I have some comments on the patch below.
Hi.
Thanks for it.
>
> Thanks,
> Kyrill
>
>> gcc/ChangeLog:
>>
>> 2018-07-18 Martin Liska <mli...@suse.cz>
>>
>> PR driver/83193
>> * common/config/aarch64/aarch64-common.c (aarch64_parse_extension):
>> Set invalid_extension when there's any.
>> (aarch64_get_all_extension_candidates): New function.
>> (aarch64_rewrite_selected_cpu): Pass NULL as new argument.
>> * config/aarch64/aarch64-protos.h
>> (aarch64_get_all_extension_candidates):
>> Declare new function.
>> * config/aarch64/aarch64.c (aarch64_parse_arch): Record
>> invalid_feature.
>> (aarch64_parse_cpu): Likewise.
>> (aarch64_print_hint_for_feature_modifier): New.
>> (aarch64_validate_mcpu): Record invalid feature modifier
>> and print hint for it.
>> (aarch64_validate_march): Likewise.
>> (aarch64_handle_attr_arch): Likewise.
>> (aarch64_handle_attr_cpu): Likewise.
>> (aarch64_handle_attr_isa_flags): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2018-07-18 Martin Liska <mli...@suse.cz>
>>
>> PR driver/83193
>> * gcc.target/aarch64/spellcheck_7.c: New test.
>> * gcc.target/aarch64/spellcheck_8.c: New test.
>> ---
>> gcc/common/config/aarch64/aarch64-common.c | 20 +++++-
>> gcc/config/aarch64/aarch64-protos.h | 4 +-
>> gcc/config/aarch64/aarch64.c | 67 +++++++++++++++----
>> .../gcc.target/aarch64/spellcheck_7.c | 11 +++
>> .../gcc.target/aarch64/spellcheck_8.c | 12 ++++
>> 5 files changed, 97 insertions(+), 17 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
>> create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c
>>
>>
>
> diff --git a/gcc/common/config/aarch64/aarch64-common.c
> b/gcc/common/config/aarch64/aarch64-common.c
> index 292fb818705..c2994514004 100644
> --- a/gcc/common/config/aarch64/aarch64-common.c
> +++ b/gcc/common/config/aarch64/aarch64-common.c
> @@ -175,7 +175,8 @@ static const struct arch_to_arch_name all_architectures[]
> =
> aarch64_parse_opt_result describing the result. */
>
> enum aarch64_parse_opt_result
> -aarch64_parse_extension (const char *str, unsigned long *isa_flags)
> +aarch64_parse_extension (const char *str, unsigned long *isa_flags,
> + char **invalid_extension)
> {
>
> Please document the new argument in the function comment. Same for all the
> other parsing functions that you extend in the patch.
> In particular, I'd like to see a comment on who allocates the memory for the
> string and who is responsible for freeing it.
>
> +/* Append all extension candidates and put them to CANDIDATES vector. */
> +
>
> Given the implementation below, how about:
> "Append all architecture extension candidates to the CANDIDATES vector." ?
>
> +void
> +aarch64_get_all_extension_candidates (auto_vec<const char *> *candidates)
> +{
> + const struct aarch64_option_extension *opt;
> + for (opt = all_extensions; opt->name != NULL; opt++)
> + candidates->safe_push (opt->name);
> +}
> +
>
> <snip>
>
> +
> +/* Print a hint with a suggestion for a feature modifier name
> + that most closely resembles what the user passed in STR. */
> +
> +void
> +aarch64_print_hint_for_feature_modifier (const char *str)
> +{
>
>
> Elsewhere in the backend and this patch as well we call them extensions
> rather than feature modifiers.
But currently following error message is printed to user:
cc1: error: invalid feature modifier in '-mcpu=thunderx+cripto'
?
I'm working on another iteration of the patch.
Martin
> Let's be consistent: aarch64_print_hint_for_extension would is my suggestion
>
> + auto_vec<const char *> candidates;
> + aarch64_get_all_extension_candidates (&candidates);
> + char *s;
> + const char *hint = candidates_list_and_hint (str, s, candidates);
> + if (hint)
> + inform (input_location, "valid arguments are: %s;"
> + " did you mean %qs?", s, hint);
> + else
> + inform (input_location, "valid arguments are: %s;", s);
> +
> + XDELETEVEC (s);
> +}
> +
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
> b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
> new file mode 100644
> index 00000000000..1350b865162
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */
>
> Another way that the extensions are used is with -mcpu. For example
> -mcpu=cortex-a57+crypto.
> So you'll need to skip if -mcpu is given as well.
> And we'll want a test for -mcpu error-checking as well.
>
> +/* { dg-options "-march=armv8-a+typo" } */
> +
> +void
> +foo ()
> +{
> +}
> +
>
