Re: PATCH: PR target/59587: cpu_names in i386.c is accessed with wrong index

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Dec 24, 2013 at 2:08 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:

> cpu_names in i386.c is only used by ix86_function_specific_print which
> accesses it with enum processor_type index. But cpu_names is defined as
> array with enum target_cpu_default index.  This patch adds processor
> names to processor_target_table and uses processor_target_table instead
> of cpu_names.  It removes cpu_names and target_cpu_default.  Tested on
> Linux/x86-64.  OK to install?
>
> Thanks.
>
>
> H.J.
> --
> 2013-12-24   H.J. Lu  <hongjiu.lu@intel.com>
>
>         PR target/59587
>         * config/i386/i386.c (struct ptt): Add a field for processor
>         name.
>         (processor_target_table): Add processor names.
>         (cpu_names): Removed.
>         (ix86_option_override_internal): Default x_ix86_tune_string
>         to processor_target_table[TARGET_CPU_DEFAULT].name.
>         (ix86_function_specific_print): Use processor_target_table
>         to print arch and tune names.
>         * config/i386/i386.h (TARGET_CPU_DEFAULT): Default to
>         PROCESSOR_GENERIC.
>         (target_cpu_default): Removed.

Those two tables were hopelessly out of sync... so, great to get rid
of one, especially since the precision of target_cpu_default enum was
never needed.

The patch is OK (with some additional cleanups, as suggested below)
for mainline and 4.8 after a couple of days in mainline.

Thanks,
Uros.

> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index ced6618..b9f0a63 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -2375,6 +2375,7 @@ static tree ix86_veclibabi_acml (enum built_in_function, tree, tree);
>  /* Processor target table, indexed by processor number */
>  struct ptt
>  {
> +  const char *const name;                      /* processor name  */
>    const struct processor_costs *cost;          /* Processor costs */
>    const int align_loop;                                /* Default alignments.  */
>    const int align_loop_max_skip;
> @@ -2385,81 +2386,30 @@ struct ptt
>
>  static const struct ptt processor_target_table[PROCESSOR_max] =

Please put "generic" entry at the top, and sort the table (and
corresponding enum processor_type in i386.h) in the same way as enum
target_cpu_default was. Please also add a comment in both places that
these tables need to be in sync.

>  {
> -  {&i386_cost, 4, 3, 4, 3, 4},
> -  {&i486_cost, 16, 15, 16, 15, 16},
> -  {&pentium_cost, 16, 7, 16, 7, 16},
> -  {&pentiumpro_cost, 16, 15, 16, 10, 16},
> -  {&geode_cost, 0, 0, 0, 0, 0},
> -  {&k6_cost, 32, 7, 32, 7, 32},
> -  {&athlon_cost, 16, 7, 16, 7, 16},
> -  {&pentium4_cost, 0, 0, 0, 0, 0},
> -  {&k8_cost, 16, 7, 16, 7, 16},
> -  {&nocona_cost, 0, 0, 0, 0, 0},
> -  /* Core 2  */
> -  {&core_cost, 16, 10, 16, 10, 16},
> -  /* Nehalem  */
> -  {&core_cost, 16, 10, 16, 10, 16},
> -  /* Sandy Bridge  */
> -  {&core_cost, 16, 10, 16, 10, 16},
> -  /* Haswell  */
> -  {&core_cost, 16, 10, 16, 10, 16},
> -  /* Bonnell  */
> -  {&atom_cost, 16, 15, 16, 7, 16},
> -  /* Silvermont  */
> -  {&slm_cost, 16, 15, 16, 7, 16},
> -  {&generic_cost, 16, 10, 16, 10, 16},
> -  {&amdfam10_cost, 32, 24, 32, 7, 32},
> -  {&bdver1_cost, 16, 10, 16, 7, 11},
> -  {&bdver2_cost, 16, 10, 16, 7, 11},
> -  {&bdver3_cost, 16, 10, 16, 7, 11},
> -  {&bdver4_cost, 16, 10, 16, 7, 11},
> -  {&btver1_cost, 16, 10, 16, 7, 11},
> -  {&btver2_cost, 16, 10, 16, 7, 11}
> -};
> -
> -static const char *const cpu_names[TARGET_CPU_DEFAULT_max] =
> -{
> -  "generic",
> -  "i386",
> -  "i486",
> -  "pentium",
> -  "pentium-mmx",
> -  "pentiumpro",
> -  "pentium2",
> -  "pentium3",
> -  "pentium4",
> -  "pentium-m",
> -  "prescott",
> -  "nocona",
> -  "core2",
> -  "corei7",
> -  "corei7-avx",
> -  "core-avx2",
> -  "atom",
> -  "slm",
> -  "nehalem",
> -  "westmere",
> -  "sandybridge",
> -  "ivybridge",
> -  "haswell",
> -  "broadwell",
> -  "bonnell",
> -  "silvermont",
> -  "intel",
> -  "geode",
> -  "k6",
> -  "k6-2",
> -  "k6-3",
> -  "athlon",
> -  "athlon-4",
> -  "k8",
> -  "amdfam10",
> -  "bdver1",
> -  "bdver2",
> -  "bdver3",
> -  "bdver4",
> -  "btver1",
> -  "btver2"
> +  {"i386", &i386_cost, 4, 3, 4, 3, 4},
> +  {"i486", &i486_cost, 16, 15, 16, 15, 16},
> +  {"pentium", &pentium_cost, 16, 7, 16, 7, 16},
> +  {"pentiumpro", &pentiumpro_cost, 16, 15, 16, 10, 16},
> +  {"geode", &geode_cost, 0, 0, 0, 0, 0},
> +  {"k6", &k6_cost, 32, 7, 32, 7, 32},
> +  {"athlon", &athlon_cost, 16, 7, 16, 7, 16},
> +  {"pentium4", &pentium4_cost, 0, 0, 0, 0, 0},
> +  {"k8", &k8_cost, 16, 7, 16, 7, 16},
> +  {"nocona", &nocona_cost, 0, 0, 0, 0, 0},
> +  {"core2", &core_cost, 16, 10, 16, 10, 16},
> +  {"nehalem", &core_cost, 16, 10, 16, 10, 16},
> +  {"sandybridge", &core_cost, 16, 10, 16, 10, 16},
> +  {"haswell", &core_cost, 16, 10, 16, 10, 16},
> +  {"bonnell", &atom_cost, 16, 15, 16, 7, 16},
> +  {"silvermont", &slm_cost, 16, 15, 16, 7, 16},
> +  {"generic", &generic_cost, 16, 10, 16, 10, 16},
> +  {"amdfam10", &amdfam10_cost, 32, 24, 32, 7, 32},
> +  {"bdver1", &bdver1_cost, 16, 10, 16, 7, 11},
> +  {"bdver2", &bdver2_cost, 16, 10, 16, 7, 11},
> +  {"bdver3", &bdver3_cost, 16, 10, 16, 7, 11},
> +  {"bdver4", &bdver4_cost, 16, 10, 16, 7, 11},
> +  {"btver1", &btver1_cost, 16, 10, 16, 7, 11},
> +  {"btver2", &btver2_cost, 16, 10, 16, 7, 11}
>  };
>
>  static bool
> @@ -3360,7 +3310,8 @@ ix86_option_override_internal (bool main_args_p,
>         opts->x_ix86_tune_string = opts->x_ix86_arch_string;
>        if (!opts->x_ix86_tune_string)
>         {
> -         opts->x_ix86_tune_string = cpu_names[TARGET_CPU_DEFAULT];
> +         opts->x_ix86_tune_string
> +           = processor_target_table[TARGET_CPU_DEFAULT].name;
>           ix86_tune_defaulted = 1;
>         }
>
> @@ -4413,17 +4364,11 @@ ix86_function_specific_print (FILE *file, int indent,
>
>    fprintf (file, "%*sarch = %d (%s)\n",
>            indent, "",
> -          ptr->arch,
> -          ((ptr->arch < TARGET_CPU_DEFAULT_max)
> -           ? cpu_names[ptr->arch]
> -           : "<unknown>"));
> +          ptr->arch, processor_target_table[ptr->arch].name);

Please add assert that "ptr->arch < PROCESSOR_max" before fprintf ...

>    fprintf (file, "%*stune = %d (%s)\n",
>            indent, "",
> -          ptr->tune,
> -          ((ptr->tune < TARGET_CPU_DEFAULT_max)
> -           ? cpu_names[ptr->tune]
> -           : "<unknown>"));
> +          ptr->tune, processor_target_table[ptr->tune].name);

... and assert for arch < PROCESSOR_max here.

>    fprintf (file, "%*sbranch_cost = %d\n", indent, "", ptr->branch_cost);
>
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index aafc1ac..076e3be 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -247,10 +247,10 @@ extern const struct processor_costs ix86_size_cost;
>
>  /* Macros used in the machine description to test the flags.  */
>
> -/* configure can arrange to make this 2, to force a 486.  */
> +/* configure can arrange to change it.  */
>
>  #ifndef TARGET_CPU_DEFAULT
> -#define TARGET_CPU_DEFAULT TARGET_CPU_DEFAULT_generic
> +#define TARGET_CPU_DEFAULT PROCESSOR_GENERIC
>  #endif
>
>  #ifndef TARGET_FPMATH_DEFAULT
> @@ -607,55 +607,6 @@ extern const char *host_detect_local_cpu (int argc, const char **argv);
>  /* Target Pragmas.  */
>  #define REGISTER_TARGET_PRAGMAS() ix86_register_pragmas ()
>
> -enum target_cpu_default
> -{
> -  TARGET_CPU_DEFAULT_generic = 0,
> -
> -  TARGET_CPU_DEFAULT_i386,
> -  TARGET_CPU_DEFAULT_i486,
> -  TARGET_CPU_DEFAULT_pentium,
> -  TARGET_CPU_DEFAULT_pentium_mmx,
> -  TARGET_CPU_DEFAULT_pentiumpro,
> -  TARGET_CPU_DEFAULT_pentium2,
> -  TARGET_CPU_DEFAULT_pentium3,
> -  TARGET_CPU_DEFAULT_pentium4,
> -  TARGET_CPU_DEFAULT_pentium_m,
> -  TARGET_CPU_DEFAULT_prescott,
> -  TARGET_CPU_DEFAULT_nocona,
> -  TARGET_CPU_DEFAULT_core2,
> -  TARGET_CPU_DEFAULT_corei7,
> -  TARGET_CPU_DEFAULT_corei7_avx,
> -  TARGET_CPU_DEFAULT_core_avx2,
> -  TARGET_CPU_DEFAULT_atom,
> -  TARGET_CPU_DEFAULT_slm,
> -  TARGET_CPU_DEFAULT_nehalem,
> -  TARGET_CPU_DEFAULT_westmere,
> -  TARGET_CPU_DEFAULT_sandybridge,
> -  TARGET_CPU_DEFAULT_ivybridge,
> -  TARGET_CPU_DEFAULT_haswell,
> -  TARGET_CPU_DEFAULT_broadwell,
> -  TARGET_CPU_DEFAULT_bonnell,
> -  TARGET_CPU_DEFAULT_silvermont,
> -  TARGET_CPU_DEFAULT_intel,
> -
> -  TARGET_CPU_DEFAULT_geode,
> -  TARGET_CPU_DEFAULT_k6,
> -  TARGET_CPU_DEFAULT_k6_2,
> -  TARGET_CPU_DEFAULT_k6_3,
> -  TARGET_CPU_DEFAULT_athlon,
> -  TARGET_CPU_DEFAULT_athlon_sse,
> -  TARGET_CPU_DEFAULT_k8,
> -  TARGET_CPU_DEFAULT_amdfam10,
> -  TARGET_CPU_DEFAULT_bdver1,
> -  TARGET_CPU_DEFAULT_bdver2,
> -  TARGET_CPU_DEFAULT_bdver3,
> -  TARGET_CPU_DEFAULT_bdver4,
> -  TARGET_CPU_DEFAULT_btver1,
> -  TARGET_CPU_DEFAULT_btver2,
> -
> -  TARGET_CPU_DEFAULT_max
> -};
> -
>  #ifndef CC1_SPEC
>  #define CC1_SPEC "%(cc1_cpu) "
>  #endif