| Message ID | 0c782751-8793-08ab-2ae7-d0ac15f502ab@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix option values for -march. | expand |
On Thu, Nov 22, 2018 at 2:43 PM Martin Liška <mliska@suse.cz> wrote: > The patch makes clear we'll not diverge number of elements in > processor_names and the corresponding enum. Plus I fixed > -march=znver2 native as valid options that were not listed. > > Patch survives tests and bootstrap on x86_64-linux-gnu. > > Ready for trunk? > Martin > > gcc/ChangeLog: > > 2018-11-22 Martin Liska <mliska@suse.cz> > > * common/config/i386/i386-common.c (processor_names): Add > static assert and add missing "znver2". > (ix86_get_valid_option_values): Add checking assert for null > values and add "native" value if feasible. > * config/i386/i386.h: Do not declare size of processor_names. > --- > gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- > gcc/config/i386/i386.h | 2 +- > 2 files changed, 23 insertions(+), 5 deletions(-) +/* Guarantee that the array is aligned with henum processor_type. */ +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) + == PROCESSOR_max)); Please use ARRAY_SIZE macro here. +#ifdef HAVE_LOCAL_CPU_DETECT + /* Add also "native" as possible value. */ + v.safe_push ("native"); +#endif "native" is processed by the driver and this option is never passed to cc1. Uros.
On Thu, Nov 22, 2018 at 2:51 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > The patch makes clear we'll not diverge number of elements in > > processor_names and the corresponding enum. Plus I fixed > > -march=znver2 native as valid options that were not listed. > > > > Patch survives tests and bootstrap on x86_64-linux-gnu. > > > > Ready for trunk? > > Martin > > > > gcc/ChangeLog: > > > > 2018-11-22 Martin Liska <mliska@suse.cz> > > > > * common/config/i386/i386-common.c (processor_names): Add > > static assert and add missing "znver2". > > (ix86_get_valid_option_values): Add checking assert for null > > values and add "native" value if feasible. > > * config/i386/i386.h: Do not declare size of processor_names. > > --- > > gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- > > gcc/config/i386/i386.h | 2 +- > > 2 files changed, 23 insertions(+), 5 deletions(-) > > +/* Guarantee that the array is aligned with henum processor_type. */ Typo above. > +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) > + == PROCESSOR_max)); > > Please use ARRAY_SIZE macro here. BTW: There is another similar table in i386.c, processor_cost_table. Can we add STATIC_ASSERT for this table, too? Uros.
On 11/22/18 2:51 PM, Uros Bizjak wrote: > On Thu, Nov 22, 2018 at 2:43 PM Martin Liška <mliska@suse.cz> wrote: > >> The patch makes clear we'll not diverge number of elements in >> processor_names and the corresponding enum. Plus I fixed >> -march=znver2 native as valid options that were not listed. >> >> Patch survives tests and bootstrap on x86_64-linux-gnu. >> >> Ready for trunk? >> Martin >> >> gcc/ChangeLog: >> >> 2018-11-22 Martin Liska <mliska@suse.cz> >> >> * common/config/i386/i386-common.c (processor_names): Add >> static assert and add missing "znver2". >> (ix86_get_valid_option_values): Add checking assert for null >> values and add "native" value if feasible. >> * config/i386/i386.h: Do not declare size of processor_names. >> --- >> gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- >> gcc/config/i386/i386.h | 2 +- >> 2 files changed, 23 insertions(+), 5 deletions(-) > > +/* Guarantee that the array is aligned with henum processor_type. */ > +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) > + == PROCESSOR_max)); > > Please use ARRAY_SIZE macro here. Fixed, it's definitely nicer! > > +#ifdef HAVE_LOCAL_CPU_DETECT > + /* Add also "native" as possible value. */ > + v.safe_push ("native"); > +#endif > > "native" is processed by the driver and this option is never passed to cc1. But it's a target common hook that is used both from driver and cc1: $ ./xgcc -B. --completion=-march=nat -march=native (Starting program: /dev/shm/objdir/gcc/xgcc -B. --completion=-march=nat) $ ./xgcc -B. --help=target | grep native ... i386 i486 i586 pentium lakemont pentium-mmx winchip-c6 winchip2 c3 samuel-2 c3-2 nehemiah c7 esther i686 pentiumpro pentium2 pentium3 pentium3m pentium-m pentium4 pentium4m prescott nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm intel geode k6 k6-2 k6-3 athlon athlon-tbird athlon-4 athlon-xp athlon-mp x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 generic native (Starting program: /dev/shm/objdir/gcc/cc1 -quiet -iprefix /dev/shm/objdir/gcc/../lib64/gcc/x86_64-pc-linux-gnu/9.0.0/ -isystem ./include -isystem ./include-fixed help-dummy -quiet -dumpbase help-dummy -mtune=generic -march=x86-64 -auxbase help-dummy --help=target -o /tmp/cc5NWl4E.s) So should be fine. Martin > > Uros. > From 7c89f9bde4d61f96005ea94055b86bc1b8e652bf Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 22 Nov 2018 13:21:44 +0100 Subject: [PATCH] Fix option values for -march. gcc/ChangeLog: 2018-11-22 Martin Liska <mliska@suse.cz> * common/config/i386/i386-common.c (processor_names): Add static assert and add missing "znver2". (ix86_get_valid_option_values): Add checking assert for null values and add "native" value if feasible. * config/i386/i386.h: Do not declare size of processor_names. --- gcc/common/config/i386/i386-common.c | 25 +++++++++++++++++++++---- gcc/config/i386/i386.h | 2 +- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/gcc/common/config/i386/i386-common.c b/gcc/common/config/i386/i386-common.c index 1017147599c..d1fb7a7464b 100644 --- a/gcc/common/config/i386/i386-common.c +++ b/gcc/common/config/i386/i386-common.c @@ -1478,7 +1478,7 @@ i386_except_unwind_info (struct gcc_options *opts) #define TARGET_SUPPORTS_SPLIT_STACK ix86_supports_split_stack /* This table must be in sync with enum processor_type in i386.h. */ -const char *const processor_names[PROCESSOR_max] = +const char *const processor_names[] = { "generic", "i386", @@ -1516,9 +1516,13 @@ const char *const processor_names[PROCESSOR_max] = "bdver4", "btver1", "btver2", - "znver1" + "znver1", + "znver2" }; +/* Guarantee that the array is aligned with henum processor_type. */ +STATIC_ASSERT (ARRAY_SIZE (processor_names) == PROCESSOR_max); + const pta processor_alias_table[] = { {"i386", PROCESSOR_I386, CPU_NONE, 0}, @@ -1734,11 +1738,24 @@ ix86_get_valid_option_values (int option_code, { case OPT_march_: for (unsigned i = 0; i < pta_size; i++) - v.safe_push (processor_alias_table[i].name); + { + const char *name = processor_alias_table[i].name; + gcc_checking_assert (name != NULL); + v.safe_push (name); + } +#ifdef HAVE_LOCAL_CPU_DETECT + /* Add also "native" as possible value. */ + v.safe_push ("native"); +#endif + break; case OPT_mtune_: for (unsigned i = 0; i < PROCESSOR_max; i++) - v.safe_push (processor_names[i]); + { + const char *name = processor_names[i]; + gcc_checking_assert (name != NULL); + v.safe_push (name); + } break; default: break; diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h index 813c86dbdfa..b9e726e3d24 100644 --- a/gcc/config/i386/i386.h +++ b/gcc/config/i386/i386.h @@ -2279,7 +2279,7 @@ enum processor_type }; #if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS) -extern const char *const processor_names[PROCESSOR_max]; +extern const char *const processor_names[]; #include "wide-int-bitmask.h"
On Thu, Nov 22, 2018 at 3:00 PM Martin Liška <mliska@suse.cz> wrote: > > On 11/22/18 2:51 PM, Uros Bizjak wrote: > > On Thu, Nov 22, 2018 at 2:43 PM Martin Liška <mliska@suse.cz> wrote: > > > >> The patch makes clear we'll not diverge number of elements in > >> processor_names and the corresponding enum. Plus I fixed > >> -march=znver2 native as valid options that were not listed. > >> > >> Patch survives tests and bootstrap on x86_64-linux-gnu. > >> > >> Ready for trunk? > >> Martin > >> > >> gcc/ChangeLog: > >> > >> 2018-11-22 Martin Liska <mliska@suse.cz> > >> > >> * common/config/i386/i386-common.c (processor_names): Add > >> static assert and add missing "znver2". > >> (ix86_get_valid_option_values): Add checking assert for null > >> values and add "native" value if feasible. > >> * config/i386/i386.h: Do not declare size of processor_names. > >> --- > >> gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- > >> gcc/config/i386/i386.h | 2 +- > >> 2 files changed, 23 insertions(+), 5 deletions(-) > > > > +/* Guarantee that the array is aligned with henum processor_type. */ > > +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) > > + == PROCESSOR_max)); > > > > Please use ARRAY_SIZE macro here. > > Fixed, it's definitely nicer! > > > > > +#ifdef HAVE_LOCAL_CPU_DETECT > > + /* Add also "native" as possible value. */ > > + v.safe_push ("native"); > > +#endif > > > > "native" is processed by the driver and this option is never passed to cc1. > > But it's a target common hook that is used both from driver and cc1: > > $ ./xgcc -B. --completion=-march=nat > -march=native > > > (Starting program: /dev/shm/objdir/gcc/xgcc -B. --completion=-march=nat) > > $ ./xgcc -B. --help=target | grep native > ... > i386 i486 i586 pentium lakemont pentium-mmx winchip-c6 winchip2 c3 samuel-2 c3-2 nehemiah c7 esther i686 pentiumpro pentium2 pentium3 pentium3m pentium-m pentium4 pentium4m prescott nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm intel geode k6 k6-2 k6-3 athlon athlon-tbird athlon-4 athlon-xp athlon-mp x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 generic native > > > > (Starting program: /dev/shm/objdir/gcc/cc1 -quiet -iprefix /dev/shm/objdir/gcc/../lib64/gcc/x86_64-pc-linux-gnu/9.0.0/ -isystem ./include -isystem ./include-fixed help-dummy -quiet -dumpbase help-dummy -mtune=generic -march=x86-64 -auxbase help-dummy --help=target -o /tmp/cc5NWl4E.s) > > So should be fine. Is -march=native accepted or rejected by cc1? It should be rejected. Uros.
On 11/22/18 3:04 PM, Uros Bizjak wrote: > On Thu, Nov 22, 2018 at 3:00 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 11/22/18 2:51 PM, Uros Bizjak wrote: >>> On Thu, Nov 22, 2018 at 2:43 PM Martin Liška <mliska@suse.cz> wrote: >>> >>>> The patch makes clear we'll not diverge number of elements in >>>> processor_names and the corresponding enum. Plus I fixed >>>> -march=znver2 native as valid options that were not listed. >>>> >>>> Patch survives tests and bootstrap on x86_64-linux-gnu. >>>> >>>> Ready for trunk? >>>> Martin >>>> >>>> gcc/ChangeLog: >>>> >>>> 2018-11-22 Martin Liska <mliska@suse.cz> >>>> >>>> * common/config/i386/i386-common.c (processor_names): Add >>>> static assert and add missing "znver2". >>>> (ix86_get_valid_option_values): Add checking assert for null >>>> values and add "native" value if feasible. >>>> * config/i386/i386.h: Do not declare size of processor_names. >>>> --- >>>> gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- >>>> gcc/config/i386/i386.h | 2 +- >>>> 2 files changed, 23 insertions(+), 5 deletions(-) >>> >>> +/* Guarantee that the array is aligned with henum processor_type. */ >>> +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) >>> + == PROCESSOR_max)); >>> >>> Please use ARRAY_SIZE macro here. >> >> Fixed, it's definitely nicer! >> >>> >>> +#ifdef HAVE_LOCAL_CPU_DETECT >>> + /* Add also "native" as possible value. */ >>> + v.safe_push ("native"); >>> +#endif >>> >>> "native" is processed by the driver and this option is never passed to cc1. >> >> But it's a target common hook that is used both from driver and cc1: >> >> $ ./xgcc -B. --completion=-march=nat >> -march=native >> >> >> (Starting program: /dev/shm/objdir/gcc/xgcc -B. --completion=-march=nat) >> >> $ ./xgcc -B. --help=target | grep native >> ... >> i386 i486 i586 pentium lakemont pentium-mmx winchip-c6 winchip2 c3 samuel-2 c3-2 nehemiah c7 esther i686 pentiumpro pentium2 pentium3 pentium3m pentium-m pentium4 pentium4m prescott nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm intel geode k6 k6-2 k6-3 athlon athlon-tbird athlon-4 athlon-xp athlon-mp x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 generic native >> >> >> >> (Starting program: /dev/shm/objdir/gcc/cc1 -quiet -iprefix /dev/shm/objdir/gcc/../lib64/gcc/x86_64-pc-linux-gnu/9.0.0/ -isystem ./include -isystem ./include-fixed help-dummy -quiet -dumpbase help-dummy -mtune=generic -march=x86-64 -auxbase help-dummy --help=target -o /tmp/cc5NWl4E.s) >> >> So should be fine. > > Is -march=native accepted or rejected by cc1? It should be rejected. It's rejected, but we provide bad hints: $ ./cc1 -march=native /tmp/arch.c cc1: error: bad value (‘native’) for ‘-march=’ switch cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native It's quite bad because we can't distinguish whether bad -march= value was passed to driver, or directly into a FE: ./cc1 -march=native2 /tmp/arch.c cc1: error: bad value (‘native2’) for ‘-march=’ switch cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native; did you mean ‘native’? ./xgcc -B. -march=native2 /tmp/arch.c cc1: error: bad value (‘native2’) for ‘-march=’ switch cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native; did you mean ‘native’? Second one should offer 'native' as possible value, but not the first one. Martin > > Uros. >
On Thu, Nov 22, 2018 at 3:22 PM Martin Liška <mliska@suse.cz> wrote: > > On 11/22/18 3:04 PM, Uros Bizjak wrote: > > On Thu, Nov 22, 2018 at 3:00 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 11/22/18 2:51 PM, Uros Bizjak wrote: > >>> On Thu, Nov 22, 2018 at 2:43 PM Martin Liška <mliska@suse.cz> wrote: > >>> > >>>> The patch makes clear we'll not diverge number of elements in > >>>> processor_names and the corresponding enum. Plus I fixed > >>>> -march=znver2 native as valid options that were not listed. > >>>> > >>>> Patch survives tests and bootstrap on x86_64-linux-gnu. > >>>> > >>>> Ready for trunk? > >>>> Martin > >>>> > >>>> gcc/ChangeLog: > >>>> > >>>> 2018-11-22 Martin Liska <mliska@suse.cz> > >>>> > >>>> * common/config/i386/i386-common.c (processor_names): Add > >>>> static assert and add missing "znver2". > >>>> (ix86_get_valid_option_values): Add checking assert for null > >>>> values and add "native" value if feasible. > >>>> * config/i386/i386.h: Do not declare size of processor_names. > >>>> --- > >>>> gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- > >>>> gcc/config/i386/i386.h | 2 +- > >>>> 2 files changed, 23 insertions(+), 5 deletions(-) > >>> > >>> +/* Guarantee that the array is aligned with henum processor_type. */ > >>> +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) > >>> + == PROCESSOR_max)); > >>> > >>> Please use ARRAY_SIZE macro here. > >> > >> Fixed, it's definitely nicer! > >> > >>> > >>> +#ifdef HAVE_LOCAL_CPU_DETECT > >>> + /* Add also "native" as possible value. */ > >>> + v.safe_push ("native"); > >>> +#endif > >>> > >>> "native" is processed by the driver and this option is never passed to cc1. > >> > >> But it's a target common hook that is used both from driver and cc1: > >> > >> $ ./xgcc -B. --completion=-march=nat > >> -march=native > >> > >> > >> (Starting program: /dev/shm/objdir/gcc/xgcc -B. --completion=-march=nat) > >> > >> $ ./xgcc -B. --help=target | grep native > >> ... > >> i386 i486 i586 pentium lakemont pentium-mmx winchip-c6 winchip2 c3 samuel-2 c3-2 nehemiah c7 esther i686 pentiumpro pentium2 pentium3 pentium3m pentium-m pentium4 pentium4m prescott nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm intel geode k6 k6-2 k6-3 athlon athlon-tbird athlon-4 athlon-xp athlon-mp x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 generic native > >> > >> > >> > >> (Starting program: /dev/shm/objdir/gcc/cc1 -quiet -iprefix /dev/shm/objdir/gcc/../lib64/gcc/x86_64-pc-linux-gnu/9.0.0/ -isystem ./include -isystem ./include-fixed help-dummy -quiet -dumpbase help-dummy -mtune=generic -march=x86-64 -auxbase help-dummy --help=target -o /tmp/cc5NWl4E.s) > >> > >> So should be fine. > > > > Is -march=native accepted or rejected by cc1? It should be rejected. > > It's rejected, but we provide bad hints: > > $ ./cc1 -march=native /tmp/arch.c > cc1: error: bad value (‘native’) for ‘-march=’ switch > cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native > > It's quite bad because we can't distinguish whether bad -march= value was passed to driver, or directly into a FE: > > ./cc1 -march=native2 /tmp/arch.c > cc1: error: bad value (‘native2’) for ‘-march=’ switch > cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native; did you mean ‘native’? > > ./xgcc -B. -march=native2 /tmp/arch.c > cc1: error: bad value (‘native2’) for ‘-march=’ switch > cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native; did you mean ‘native’? > > Second one should offer 'native' as possible value, but not the first one. Yes, but this precedes your patch. So the situation is the same... LGTM then, but please also add static assert to processor_cost _table. Uros.
On 11/22/18 3:28 PM, Uros Bizjak wrote: > On Thu, Nov 22, 2018 at 3:22 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 11/22/18 3:04 PM, Uros Bizjak wrote: >>> On Thu, Nov 22, 2018 at 3:00 PM Martin Liška <mliska@suse.cz> wrote: >>>> >>>> On 11/22/18 2:51 PM, Uros Bizjak wrote: >>>>> On Thu, Nov 22, 2018 at 2:43 PM Martin Liška <mliska@suse.cz> wrote: >>>>> >>>>>> The patch makes clear we'll not diverge number of elements in >>>>>> processor_names and the corresponding enum. Plus I fixed >>>>>> -march=znver2 native as valid options that were not listed. >>>>>> >>>>>> Patch survives tests and bootstrap on x86_64-linux-gnu. >>>>>> >>>>>> Ready for trunk? >>>>>> Martin >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> 2018-11-22 Martin Liska <mliska@suse.cz> >>>>>> >>>>>> * common/config/i386/i386-common.c (processor_names): Add >>>>>> static assert and add missing "znver2". >>>>>> (ix86_get_valid_option_values): Add checking assert for null >>>>>> values and add "native" value if feasible. >>>>>> * config/i386/i386.h: Do not declare size of processor_names. >>>>>> --- >>>>>> gcc/common/config/i386/i386-common.c | 26 ++++++++++++++++++++++---- >>>>>> gcc/config/i386/i386.h | 2 +- >>>>>> 2 files changed, 23 insertions(+), 5 deletions(-) >>>>> >>>>> +/* Guarantee that the array is aligned with henum processor_type. */ >>>>> +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) >>>>> + == PROCESSOR_max)); >>>>> >>>>> Please use ARRAY_SIZE macro here. >>>> >>>> Fixed, it's definitely nicer! >>>> >>>>> >>>>> +#ifdef HAVE_LOCAL_CPU_DETECT >>>>> + /* Add also "native" as possible value. */ >>>>> + v.safe_push ("native"); >>>>> +#endif >>>>> >>>>> "native" is processed by the driver and this option is never passed to cc1. >>>> >>>> But it's a target common hook that is used both from driver and cc1: >>>> >>>> $ ./xgcc -B. --completion=-march=nat >>>> -march=native >>>> >>>> >>>> (Starting program: /dev/shm/objdir/gcc/xgcc -B. --completion=-march=nat) >>>> >>>> $ ./xgcc -B. --help=target | grep native >>>> ... >>>> i386 i486 i586 pentium lakemont pentium-mmx winchip-c6 winchip2 c3 samuel-2 c3-2 nehemiah c7 esther i686 pentiumpro pentium2 pentium3 pentium3m pentium-m pentium4 pentium4m prescott nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm intel geode k6 k6-2 k6-3 athlon athlon-tbird athlon-4 athlon-xp athlon-mp x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 generic native >>>> >>>> >>>> >>>> (Starting program: /dev/shm/objdir/gcc/cc1 -quiet -iprefix /dev/shm/objdir/gcc/../lib64/gcc/x86_64-pc-linux-gnu/9.0.0/ -isystem ./include -isystem ./include-fixed help-dummy -quiet -dumpbase help-dummy -mtune=generic -march=x86-64 -auxbase help-dummy --help=target -o /tmp/cc5NWl4E.s) >>>> >>>> So should be fine. >>> >>> Is -march=native accepted or rejected by cc1? It should be rejected. >> >> It's rejected, but we provide bad hints: >> >> $ ./cc1 -march=native /tmp/arch.c >> cc1: error: bad value (‘native’) for ‘-march=’ switch >> cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native >> >> It's quite bad because we can't distinguish whether bad -march= value was passed to driver, or directly into a FE: >> >> ./cc1 -march=native2 /tmp/arch.c >> cc1: error: bad value (‘native2’) for ‘-march=’ switch >> cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native; did you mean ‘native’? >> >> ./xgcc -B. -march=native2 /tmp/arch.c >> cc1: error: bad value (‘native2’) for ‘-march=’ switch >> cc1: note: valid arguments to ‘-march=’ switch are: nocona core2 nehalem corei7 westmere sandybridge corei7-avx ivybridge core-avx-i haswell core-avx2 broadwell skylake skylake-avx512 cannonlake icelake-client icelake-server bonnell atom silvermont slm goldmont goldmont-plus tremont knl knm x86-64 eden-x2 nano nano-1000 nano-2000 nano-3000 nano-x2 eden-x4 nano-x4 k8 k8-sse3 opteron opteron-sse3 athlon64 athlon64-sse3 athlon-fx amdfam10 barcelona bdver1 bdver2 bdver3 bdver4 znver1 znver2 btver1 btver2 native; did you mean ‘native’? >> >> Second one should offer 'native' as possible value, but not the first one. > > Yes, but this precedes your patch. So the situation is the same... > > LGTM then, but please also add static assert to processor_cost _table. > > Uros. > Sure, there's final version of the patch I'm going to install. Martin From 80ed88cc8a087c41de4e576257b8c9d84e434902 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 22 Nov 2018 13:21:44 +0100 Subject: [PATCH] Fix option values for -march. gcc/ChangeLog: 2018-11-22 Martin Liska <mliska@suse.cz> * common/config/i386/i386-common.c (processor_names): Add static assert and add missing "znver2". (ix86_get_valid_option_values): Add checking assert for null values and add "native" value if feasible. * config/i386/i386.h: Do not declare size of processor_names. * common/config/i386/i386-common.c: * config/i386/i386.c: Add static assert for size of processor_cost_table. --- gcc/common/config/i386/i386-common.c | 25 +++++++++++++++++++++---- gcc/config/i386/i386.c | 5 ++++- gcc/config/i386/i386.h | 2 +- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/gcc/common/config/i386/i386-common.c b/gcc/common/config/i386/i386-common.c index 1017147599c..4238b432431 100644 --- a/gcc/common/config/i386/i386-common.c +++ b/gcc/common/config/i386/i386-common.c @@ -1478,7 +1478,7 @@ i386_except_unwind_info (struct gcc_options *opts) #define TARGET_SUPPORTS_SPLIT_STACK ix86_supports_split_stack /* This table must be in sync with enum processor_type in i386.h. */ -const char *const processor_names[PROCESSOR_max] = +const char *const processor_names[] = { "generic", "i386", @@ -1516,9 +1516,13 @@ const char *const processor_names[PROCESSOR_max] = "bdver4", "btver1", "btver2", - "znver1" + "znver1", + "znver2" }; +/* Guarantee that the array is aligned with enum processor_type. */ +STATIC_ASSERT (ARRAY_SIZE (processor_names) == PROCESSOR_max); + const pta processor_alias_table[] = { {"i386", PROCESSOR_I386, CPU_NONE, 0}, @@ -1734,11 +1738,24 @@ ix86_get_valid_option_values (int option_code, { case OPT_march_: for (unsigned i = 0; i < pta_size; i++) - v.safe_push (processor_alias_table[i].name); + { + const char *name = processor_alias_table[i].name; + gcc_checking_assert (name != NULL); + v.safe_push (name); + } +#ifdef HAVE_LOCAL_CPU_DETECT + /* Add also "native" as possible value. */ + v.safe_push ("native"); +#endif + break; case OPT_mtune_: for (unsigned i = 0; i < PROCESSOR_max; i++) - v.safe_push (processor_names[i]); + { + const char *name = processor_names[i]; + gcc_checking_assert (name != NULL); + v.safe_push (name); + } break; default: break; diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 21eb6a2d42c..2f0d531427b 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -831,7 +831,7 @@ static tree ix86_veclibabi_svml (combined_fn, tree, tree); static tree ix86_veclibabi_acml (combined_fn, tree, tree); /* This table must be in sync with enum processor_type in i386.h. */ -static const struct processor_costs *processor_cost_table[PROCESSOR_max] = +static const struct processor_costs *processor_cost_table[] = { &generic_cost, &i386_cost, @@ -872,6 +872,9 @@ static const struct processor_costs *processor_cost_table[PROCESSOR_max] = &znver1_cost, &znver2_cost }; + +/* Guarantee that the array is aligned with enum processor_type. */ +STATIC_ASSERT (ARRAY_SIZE (processor_cost_table) == PROCESSOR_max); static unsigned int rest_of_handle_insert_vzeroupper (void) diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h index 813c86dbdfa..b9e726e3d24 100644 --- a/gcc/config/i386/i386.h +++ b/gcc/config/i386/i386.h @@ -2279,7 +2279,7 @@ enum processor_type }; #if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS) -extern const char *const processor_names[PROCESSOR_max]; +extern const char *const processor_names[]; #include "wide-int-bitmask.h"
diff --git a/gcc/common/config/i386/i386-common.c b/gcc/common/config/i386/i386-common.c index 1017147599c..3deac94a879 100644 --- a/gcc/common/config/i386/i386-common.c +++ b/gcc/common/config/i386/i386-common.c @@ -1478,7 +1478,7 @@ i386_except_unwind_info (struct gcc_options *opts) #define TARGET_SUPPORTS_SPLIT_STACK ix86_supports_split_stack /* This table must be in sync with enum processor_type in i386.h. */ -const char *const processor_names[PROCESSOR_max] = +const char *const processor_names[] = { "generic", "i386", @@ -1516,9 +1516,14 @@ const char *const processor_names[PROCESSOR_max] = "bdver4", "btver1", "btver2", - "znver1" + "znver1", + "znver2" }; +/* Guarantee that the array is aligned with henum processor_type. */ +STATIC_ASSERT ((sizeof (processor_names) / sizeof (processor_names[0]) + == PROCESSOR_max)); + const pta processor_alias_table[] = { {"i386", PROCESSOR_I386, CPU_NONE, 0}, @@ -1734,11 +1739,24 @@ ix86_get_valid_option_values (int option_code, { case OPT_march_: for (unsigned i = 0; i < pta_size; i++) - v.safe_push (processor_alias_table[i].name); + { + const char *name = processor_alias_table[i].name; + gcc_checking_assert (name != NULL); + v.safe_push (name); + } +#ifdef HAVE_LOCAL_CPU_DETECT + /* Add also "native" as possible value. */ + v.safe_push ("native"); +#endif + break; case OPT_mtune_: for (unsigned i = 0; i < PROCESSOR_max; i++) - v.safe_push (processor_names[i]); + { + const char *name = processor_names[i]; + gcc_checking_assert (name != NULL); + v.safe_push (name); + } break; default: break; diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h index 813c86dbdfa..b9e726e3d24 100644 --- a/gcc/config/i386/i386.h +++ b/gcc/config/i386/i386.h @@ -2279,7 +2279,7 @@ enum processor_type }; #if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS) -extern const char *const processor_names[PROCESSOR_max]; +extern const char *const processor_names[]; #include "wide-int-bitmask.h"