On Mon, 2016-09-05 at 21:00 +0200, Uros Bizjak wrote:
> On Mon, Sep 5, 2016 at 7:25 PM, Jakub Jelinek <ja...@redhat.com>
> wrote:
> > Hi!
> >
> > While most of the i386.opt -m....= options have enum args and thus
> > cmdline_handle_error handles those, -march=/-mtune=/-m*-strategy=
> > (and also
> > -mrecip=) don't use that, with the CPU strings being maintained
> > inside of a
> > function rather than in some *.def file that could be also sourced
> > into the
> > *.opt or something (and similarly for the strategies).
> >
> > This patch adds inform calls that handle those similarly to what
> > cmdline_handle_error does for the options with enum values.
> > In addition, it adds %qs instead of %s in a couple of spaces, and
> > stops reporting incorrect attribute option("march=...") when it is
> > target("march=...") etc.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk?
> >
> > 2016-09-05 Jakub Jelinek <ja...@redhat.com>
> >
> > PR middle-end/77475
> > * config/i386/i386.c: Include spellcheck.h.
> > (ix86_parse_stringop_strategy_string): Simplify, use %qs
> > instead of %s
> > where desirable, use argument instead of arg in the
> > diagnostic
> > wording, add list of supported strategies and spellcheck
> > hint.
> > (ix86_option_override_internal): Emit target("m...")
> > instead of
> > option("m...") in the diagnostic. Use %qs instead of %s in
> > invalid
> > -march/-mtune option diagnostic. Add list of supported
> > arches/tunings
> > and spellcheck hint.
> >
> > * gcc.target/i386/pr65990.c: Adjust expected diagnostics.
>
> OK as far as x86 target is concerned, but please allow a day for
> David
> to eventually comment on the implementation.
The calls into spellcheck.h are minimal and look reasonable (I can't
really comment on the x86 aspects). So I have little to add beyond the
cleanups that Manu already observed.
One thing: shouldn't this have testcases that give test coverage for
emitting hints for -march and -mtune? Something like
gcc/testsuite/gcc.dg/spellcheck-options-11.c
(though obviously the new ones would be target-specific).
> Thanks,
> Uros.
>
> > --- gcc/config/i386/i386.c.jj 2016-09-05 12:41:03.000000000 +0200
> > +++ gcc/config/i386/i386.c 2016-09-05 16:44:45.184981211 +0200
> > @@ -76,6 +76,7 @@ along with GCC; see the file COPYING3.
> > #include "case-cfn-macros.h"
> > #include "regrename.h"
> > #include "dojump.h"
> > +#include "spellcheck.h"
> >
> > /* This file should be included last. */
> > #include "target-def.h"
> > @@ -4533,19 +4534,19 @@ ix86_parse_stringop_strategy_string (cha
> > next_range_str = strchr (curr_range_str, ',');
> > if (next_range_str)
> > *next_range_str++ = '\0';
> > + const char *opt
> > + = is_memset ? "-mmemset_strategy=" : "-mmemcpy_strategy=";
> >
> > if (3 != sscanf (curr_range_str, "%20[^:]:%d:%10s",
> > alg_name, &maxs, align))
> > {
> > - error ("wrong arg %s to option %s", curr_range_str,
> > - is_memset ? "-mmemset_strategy=" : "
> > -mmemcpy_strategy=");
> > + error ("wrong argument %qs to option %qs",
> > curr_range_str, opt);
> > return;
> > }
> >
> > if (n > 0 && (maxs < (input_ranges[n - 1].max + 1) && maxs
> > != -1))
> > {
> > - error ("size ranges of option %s should be increasing",
> > - is_memset ? "-mmemset_strategy=" : "
> > -mmemcpy_strategy=");
> > + error ("size ranges of option %qs should be increasing",
> > opt);
> > return;
> > }
> >
> > @@ -4555,9 +4556,35 @@ ix86_parse_stringop_strategy_string (cha
> >
> > if (i == last_alg)
> > {
> > - error ("wrong stringop strategy name %s specified for
> > option %s",
> > - alg_name,
> > - is_memset ? "-mmemset_strategy=" : "
> > -mmemcpy_strategy=");
> > + error ("wrong stringop strategy name %qs specified for
> > option %s",
> > + alg_name, opt);
> > +
> > + size_t len = 0;
> > + for (i = 0; i < last_alg; i++)
> > + len += strlen (stringop_alg_names[i]) + 1;
> > +
> > + char *s, *p;
> > + auto_vec <const char *> candidates;
> > + s = XALLOCAVEC (char, len);
> > + p = s;
> > + for (i = 0; i < last_alg; i++)
> > + if ((stringop_alg) i != rep_prefix_8_byte ||
> > TARGET_64BIT)
> > + {
> > + size_t arglen = strlen (stringop_alg_names[i]);
> > + memcpy (p, stringop_alg_names[i], arglen);
> > + p[arglen] = ' ';
> > + p += arglen + 1;
> > + candidates.safe_push (stringop_alg_names[i]);
> > + }
> > + p[-1] = 0;
> > + const char *hint = find_closest_string (alg_name,
> > &candidates);
> > + if (hint)
> > + inform (input_location,
> > + "valid arguments to %qs are: %s; did you mean
> > %qs?",
> > + opt, s, hint);
> > + else
> > + inform (input_location, "valid arguments to %qs are:
> > %s",
> > + opt, s);
> > return;
> > }
> >
> > @@ -4565,10 +4592,8 @@ ix86_parse_stringop_strategy_string (cha
> > && !TARGET_64BIT)
> > {
> > /* rep; movq isn't available in 32-bit code. */
> > - error ("stringop strategy name %s specified for option %s
> > "
> > - "not supported for 32-bit code",
> > - alg_name,
> > - is_memset ? "-mmemset_strategy=" : "
> > -mmemcpy_strategy=");
> > + error ("stringop strategy name %qs specified for option
> > %qs "
> > + "not supported for 32-bit code", alg_name, opt);
> > return;
> > }
> >
> > @@ -4580,8 +4605,7 @@ ix86_parse_stringop_strategy_string (cha
> > input_ranges[n].noalign = true;
> > else
> > {
> > - error ("unknown alignment %s specified for option %s",
> > - align, is_memset ? "-mmemset_strategy=" : "
> > -mmemcpy_strategy=");
> > + error ("unknown alignment %qs specified for option %qs",
> > align, opt);
> > return;
> > }
> > n++;
> > @@ -5041,7 +5065,7 @@ ix86_option_override_internal (bool main
> > }
> > else
> > {
> > - prefix = "option(\"";
> > + prefix = "target(\"";
> > suffix = "\")";
> > sw = "attribute";
> > }
> > @@ -5480,8 +5504,41 @@ ix86_option_override_internal (bool main
> > error ("intel CPU can be used only for %stune=%s %s",
> > prefix, suffix, sw);
> > else if (i == pta_size)
> > - error ("bad value (%s) for %sarch=%s %s",
> > - opts->x_ix86_arch_string, prefix, suffix, sw);
> > + {
> > + error ("bad value (%qs) for %<%sarch=%s%> %s",
> > + opts->x_ix86_arch_string, prefix, suffix, sw);
> > +
> > + size_t len = 0;
> > + for (i = 0; i < pta_size; i++)
> > + len += strlen (processor_alias_table[i].name) + 1;
> > +
> > + char *s, *p;
> > + auto_vec <const char *> candidates;
> > + s = XALLOCAVEC (char, len);
> > + p = s;
> > + for (i = 0; i < pta_size; i++)
> > + if (strcmp (processor_alias_table[i].name, "generic")
> > + && strcmp (processor_alias_table[i].name, "intel")
> > + && (!TARGET_64BIT_P (opts->x_ix86_isa_flags)
> > + || (processor_alias_table[i].flags & PTA_64BIT)))
> > + {
> > + size_t arglen = strlen (processor_alias_table[i].name);
> > + memcpy (p, processor_alias_table[i].name, arglen);
> > + p[arglen] = ' ';
> > + p += arglen + 1;
> > + candidates.safe_push (processor_alias_table[i].name);
> > + }
> > + p[-1] = 0;
> > + const char *hint
> > + = find_closest_string (opts->x_ix86_arch_string,
> > &candidates);
> > + if (hint)
> > + inform (input_location,
> > + "valid arguments to %<%sarch=%s%> %s are: %s; "
> > + "did you mean %qs?", prefix, suffix, sw, s, hint);
> > + else
> > + inform (input_location, "valid arguments to %<%sarch=%s%>
> > %s are: %s",
> > + prefix, suffix, sw, s);
> > + }
> >
> > ix86_arch_mask = 1u << ix86_arch;
> > for (i = 0; i < X86_ARCH_LAST; ++i)
> > @@ -5523,8 +5580,40 @@ ix86_option_override_internal (bool main
> > }
> >
> > if (ix86_tune_specified && i == pta_size)
> > - error ("bad value (%s) for %stune=%s %s",
> > - opts->x_ix86_tune_string, prefix, suffix, sw);
> > + {
> > + error ("bad value (%qs) for %<%stune=%s%> %s",
> > + opts->x_ix86_tune_string, prefix, suffix, sw);
> > +
> > + size_t len = 0;
> > + for (i = 0; i < pta_size; i++)
> > + len += strlen (processor_alias_table[i].name) + 1;
> > +
> > + char *s, *p;
> > + auto_vec <const char *> candidates;
> > + s = XALLOCAVEC (char, len);
> > + p = s;
> > + for (i = 0; i < pta_size; i++)
> > + if (!TARGET_64BIT_P (opts->x_ix86_isa_flags)
> > + || (processor_alias_table[i].flags & PTA_64BIT))
> > + {
> > + size_t arglen = strlen (processor_alias_table[i].name);
> > + memcpy (p, processor_alias_table[i].name, arglen);
> > + p[arglen] = ' ';
> > + p += arglen + 1;
> > + candidates.safe_push (processor_alias_table[i].name);
> > + }
> > + p[-1] = 0;
> > + const char *hint
> > + = find_closest_string (opts->x_ix86_tune_string,
> > &candidates);
> > + if (hint)
> > + inform (input_location,
> > + "valid arguments to %<%stune=%s%> %s are: %s; "
> > + "did you mean %qs?", prefix, suffix, sw, s, hint);
> > + else
> > + inform (input_location, "valid arguments to %<%stune=%s%>
> > %s are: %s",
> > + prefix, suffix, sw, s);
> > +
> > + }
> >
> > set_ix86_tune_features (ix86_tune, opts->x_ix86_dump_tunes);
> >
> > --- gcc/testsuite/gcc.target/i386/pr65990.c.jj 2016-05-22
> > 12:20:14.000000000 +0200
> > +++ gcc/testsuite/gcc.target/i386/pr65990.c 2016-09-05
> > 19:00:48.000000000 +0200
> > @@ -1,7 +1,7 @@
> > /* { dg-do compile } */
> > /* { dg-options "-mtune=btver2 -mmemcpy-strategy=rep_8byte:
> > -1:noalign" }
> >
> > -/* { dg-error "stringop strategy name rep_8byte specified for
> > option -mmemcpy_strategy= not supported for 32-bit code" "" {
> > target ia32 } 0 } */
> > +/* { dg-error "stringop strategy name 'rep_8byte' specified for
> > option '-mmemcpy_strategy=' not supported for 32-bit code" "" {
> > target ia32 } 0 } */
> >
> > struct U9
> > {
> >
> > Jakub
