On 11/22/18 3:28 PM, Uros Bizjak wrote:
> On Thu, Nov 22, 2018 at 3:22 PM Martin Liška <mli...@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 <mli...@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 <mli...@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 <mli...@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 <mli...@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 <mli...@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"
--
2.19.1
