Re: [PATCH] Provide extension hint for aarch64 target (PR driver/83193).

[Martin Liška <mliska at suse dot cz>]

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  <mliska@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  <mliska@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 ()
> +{
> +}
> +
>