diff mbox

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

Message ID 20131225213156.GA24342@gmail.com
State New
Headers show

Commit Message

H.J. Lu Dec. 25, 2013, 9:31 p.m. UTC 
On Wed, Dec 25, 2013 at 10:03:45PM +0100, Uros Bizjak wrote:
> On Wed, Dec 25, 2013 at 10:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Wed, Dec 25, 2013 at 12:49 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> >> On Tue, Dec 24, 2013 at 7:03 PM, H.J. Lu <hjl.tools@gmail.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?
> >>>>>>>
> >>>>>>> Wait a moment,
> >>>>>>>
> >>>>>>> it looks to me that TARGET_CPU_DEFAULT has to be synchronized with
> >>>>>>> const processor_alias_table, so we are able to define various ISA
> >>>>>>> extensions by selecting TARGET_CPU_*. The TARGET_CPU_DEFAULT can then
> >>>>>>
> >>>>>> TARGET_CPU_DEFAULT sets the default -mtune=, not -march=.
> >>>>>>
> >>>>>>> be used to select extensions in the same way as PROCESSOR_* selects
> >>>>>>> tuning for certain processor.
> >>>>>>
> >>>>>> It has been like this for a long time.  For x86, TARGET_CPU_DEFAULT
> >>>>>> isn't defined no matter which configure options are used.  We can
> >>>>>> change config.gcc to set TARGET_CPU_DEFAULT to proper PROCESSOR_XXX or
> >>>>>> set it to a string "xxx" for processor "xxx".
> >>>>>> But GCC driver always passes -march=/-mtune= to toplev.c so that
> >>>>>> TARGET_CPU_DEFAULT is normally used.
> >>>>
> >>>> I meant to say "TARGET_CPU_DEFAULT isn't normally used."
> >>>>
> >>>>>
> >>>>> Let me rethink this a bit, please do not commit the patch.
> >>>>>
> >>>
> >>> TARGET_CPU_DEFAULT is left over for 32-bit target before --with-arch=
> >>> and --with-cpu= were added.  Today, -mtune=xxx -march=xxx are
> >>> always passed to cc1 by GCC driver.  If cc1 is run by hand and
> >>> -mtune=xxx -march=xxx aren't passed to cc1, we should do
> >>>
> >>> 1. For 64-bit, it should be the same as -mtune=generic -march=x86_64
> >>> are passed.
> >>> 2. For 32-bit, it should be the same as -mtune=cpu -march=cpu are
> >>> passed, where "cpu" is the target cpu used to configure GCC,
> >>> like i386 in i386-linux, i486 in i486-linux, .... But there is no i786
> >>> cpu.  i786 is treated as i686.  If SUBTARGET32_DEFAULT_CPU
> >>> is defined, it should be the same -mtune=SUBTARGET32_DEFAULT_CPU
> >>> -march=SUBTARGET32_DEFAULT_CPU.
> >>>
> >>> Here is the patch to implement this.
> >>
> >> Let's do one step at a time. So, let's split the patch back to target/59587 fix:
> >>
> >> -#define SUBTARGET32_DEFAULT_CPU "i386"
> >> +# ifdef TARGET_CPU_DEFAULT
> >> +#  define SUBTARGET32_DEFAULT_CPU
> >> processor_target_table[TARGET_CPU_DEFAULT].name
> >> +# else
> >> +#  define SUBTARGET32_DEFAULT_CPU "i386"
> >> +# endif
> >>
> >> Not in this patch ...
> >>
> >> -      opts->x_ix86_tune_string = cpu_names[TARGET_CPU_DEFAULT];
> >> +      opts->x_ix86_tune_string
> >> +#ifdef TARGET_CPU_DEFAULT
> >> +        = TARGET_64BIT_P (opts->x_ix86_isa_flags)
> >> +          ? "generic" : processor_target_table[TARGET_CPU_DEFAULT].name;
> >> +#else
> >> +        = "generic";
> >> +#endif
> >>
> >> Please split these to another patch.
> >>
> >> +  gcc_assert (ptr->arch < PROCESSOR_max);
> >>    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);
> >>
> >> I think we should leave the original, with <unknown>, comparing with
> >> PROCESSOR_max and looking into processor_target_table for the name. We
> >> can remove the assert.
> >>
> >> +  gcc_assert (ptr->tune < PROCESSOR_max);
> >>    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);
> >
> > ptr->tune and ptr->arch are set by
> >
> >   ptr->arch = ix86_arch;
> >   ptr->tune = ix86_tune;
> >
> > Both ix86_arch and  ix86_tune are enum processor_type.  If  ix86_arch
> > or  ix86_tune >= PROCESSOR_max, x86 backend won't work at all.
> 
> OK, let's leave assert then.
> 

Here is the updated patch without configure changes. OK for trunk and
4.8 branch?

Thanks.


H.J.
---
2013-12-25   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): Sync with processor_type.  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): Assert arch and tune <
	PROCESSOR_max.  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.
	(processor_type): Reordered.

Comments

Uros Bizjak Dec. 25, 2013, 10:32 p.m. UTC | #1 
On Wed, Dec 25, 2013 at 10:31 PM, H.J. Lu <hjl.tools@gmail.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?
>> >>>>>>>
>> >>>>>>> Wait a moment,
>> >>>>>>>
>> >>>>>>> it looks to me that TARGET_CPU_DEFAULT has to be synchronized with
>> >>>>>>> const processor_alias_table, so we are able to define various ISA
>> >>>>>>> extensions by selecting TARGET_CPU_*. The TARGET_CPU_DEFAULT can then
>> >>>>>>
>> >>>>>> TARGET_CPU_DEFAULT sets the default -mtune=, not -march=.
>> >>>>>>
>> >>>>>>> be used to select extensions in the same way as PROCESSOR_* selects
>> >>>>>>> tuning for certain processor.
>> >>>>>>
>> >>>>>> It has been like this for a long time.  For x86, TARGET_CPU_DEFAULT
>> >>>>>> isn't defined no matter which configure options are used.  We can
>> >>>>>> change config.gcc to set TARGET_CPU_DEFAULT to proper PROCESSOR_XXX or
>> >>>>>> set it to a string "xxx" for processor "xxx".
>> >>>>>> But GCC driver always passes -march=/-mtune= to toplev.c so that
>> >>>>>> TARGET_CPU_DEFAULT is normally used.
>> >>>>
>> >>>> I meant to say "TARGET_CPU_DEFAULT isn't normally used."
>> >>>>
>> >>>>>
>> >>>>> Let me rethink this a bit, please do not commit the patch.
>> >>>>>
>> >>>
>> >>> TARGET_CPU_DEFAULT is left over for 32-bit target before --with-arch=
>> >>> and --with-cpu= were added.  Today, -mtune=xxx -march=xxx are
>> >>> always passed to cc1 by GCC driver.  If cc1 is run by hand and
>> >>> -mtune=xxx -march=xxx aren't passed to cc1, we should do
>> >>>
>> >>> 1. For 64-bit, it should be the same as -mtune=generic -march=x86_64
>> >>> are passed.
>> >>> 2. For 32-bit, it should be the same as -mtune=cpu -march=cpu are
>> >>> passed, where "cpu" is the target cpu used to configure GCC,
>> >>> like i386 in i386-linux, i486 in i486-linux, .... But there is no i786
>> >>> cpu.  i786 is treated as i686.  If SUBTARGET32_DEFAULT_CPU
>> >>> is defined, it should be the same -mtune=SUBTARGET32_DEFAULT_CPU
>> >>> -march=SUBTARGET32_DEFAULT_CPU.
>> >>>
>> >>> Here is the patch to implement this.
>> >>
>> >> Let's do one step at a time. So, let's split the patch back to target/59587 fix:

> 2013-12-25   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): Sync with processor_type.  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): Assert arch and tune <
>         PROCESSOR_max.  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.
>         (processor_type): Reordered.

OK for mainline and for 4.8 after a few days in mainline.

Thanks,
Uros.
diff mbox

Patch

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index f5d9ce5..a58a8ae 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;
@@ -2383,83 +2384,33 @@  struct ptt
   const int align_func;
 };
 
+/* This table must be in sync with enum processor_type in i386.h.  */ 
 static const struct ptt processor_target_table[PROCESSOR_max] =
 {
-  {&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"
+  {"generic", &generic_cost, 16, 10, 16, 10, 16},
+  {"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},
+  {"pentium4", &pentium4_cost, 0, 0, 0, 0, 0},
+  {"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},
+  {"geode", &geode_cost, 0, 0, 0, 0, 0},
+  {"k6", &k6_cost, 32, 7, 32, 7, 32},
+  {"athlon", &athlon_cost, 16, 7, 16, 7, 16},
+  {"k8", &k8_cost, 16, 7, 16, 7, 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 +3311,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;
 	}
 
@@ -4411,19 +4363,15 @@  ix86_function_specific_print (FILE *file, int indent,
     = ix86_target_string (ptr->x_ix86_isa_flags, ptr->x_target_flags,
 			  NULL, NULL, ptr->x_ix86_fpmath, false);
 
+  gcc_assert (ptr->arch < PROCESSOR_max);
   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);
 
+  gcc_assert (ptr->tune < PROCESSOR_max);
   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);
 
   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..5976435 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
@@ -2213,19 +2164,17 @@  do {									\
    with x86-64 medium memory model */
 #define DEFAULT_LARGE_SECTION_THRESHOLD 65536
 
-/* Which processor to tune code generation for.  */
+/* Which processor to tune code generation for.  These must be in sync
+   with processor_target_table in i386.c.  */ 
 
 enum processor_type
 {
-  PROCESSOR_I386 = 0,			/* 80386 */
+  PROCESSOR_GENERIC = 0,
+  PROCESSOR_I386,			/* 80386 */
   PROCESSOR_I486,			/* 80486DX, 80486SX, 80486DX[24] */
   PROCESSOR_PENTIUM,
   PROCESSOR_PENTIUMPRO,
-  PROCESSOR_GEODE,
-  PROCESSOR_K6,
-  PROCESSOR_ATHLON,
   PROCESSOR_PENTIUM4,
-  PROCESSOR_K8,
   PROCESSOR_NOCONA,
   PROCESSOR_CORE2,
   PROCESSOR_NEHALEM,
@@ -2233,7 +2182,10 @@  enum processor_type
   PROCESSOR_HASWELL,
   PROCESSOR_BONNELL,
   PROCESSOR_SILVERMONT,
-  PROCESSOR_GENERIC,
+  PROCESSOR_GEODE,
+  PROCESSOR_K6,
+  PROCESSOR_ATHLON,
+  PROCESSOR_K8,
   PROCESSOR_AMDFAM10,
   PROCESSOR_BDVER1,
   PROCESSOR_BDVER2,