diff mbox

Define HAS_CPUID/HAS_I586/HAS_I686 from -march=

Message ID 20150817231005.GA24205@intel.com
State Superseded
Headers

Commit Message

Lu, Hongjiu Aug. 17, 2015, 11:10 p.m. UTC 
  cpuid, i586 and i686 instructions are available if the processor
specified by -march= supports them.  We can use this information
to determine whether those instructions can be used safely.

OK for master?

H.J.
---
	* sysdeps/x86/cpu-features.c (init_cpu_features): Check
	whether cpuid is available only if HAS_CPUID is 0.
	* sysdeps/x86/cpu-features.h (HAS_CPUID): New.
	(HAS_I586): Likewise.
	(HAS_I686): Likewise.
---
 sysdeps/x86/cpu-features.c |  4 ++--
 sysdeps/x86/cpu-features.h | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

Comments

Ondrej Bilka Aug. 18, 2015, 5:24 a.m. UTC | #1 
On Mon, Aug 17, 2015 at 04:10:05PM -0700, H.J. Lu wrote:
> cpuid, i586 and i686 instructions are available if the processor
> specified by -march= supports them.  We can use this information
> to determine whether those instructions can be used safely.
> 
> OK for master?
>
Patch itself looks ok. It fixes theoretical problem that if there would
be new processor without cpuid it would cause problem.

But could you explain rationale? Here you replaced one like with
equivalent longer macro and HAS_I586 is never used. Do you plan followup
patches that use that?
H.J. Lu Aug. 18, 2015, 1:34 p.m. UTC | #2 
On Mon, Aug 17, 2015 at 10:24 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
> On Mon, Aug 17, 2015 at 04:10:05PM -0700, H.J. Lu wrote:
>> cpuid, i586 and i686 instructions are available if the processor
>> specified by -march= supports them.  We can use this information
>> to determine whether those instructions can be used safely.
>>
>> OK for master?
>>
> Patch itself looks ok. It fixes theoretical problem that if there would
> be new processor without cpuid it would cause problem.
>
> But could you explain rationale? Here you replaced one like with
> equivalent longer macro and HAS_I586 is never used. Do you plan followup
> patches that use that?

I will use HAS_I586 and HAS_I686 in multi-arch for i486 and i586
I am working on.
Mike Frysinger Aug. 18, 2015, 2:32 p.m. UTC | #3 
On 17 Aug 2015 16:10, H.J. Lu wrote:
> +#ifdef __x86_64__
> +# define HAS_CPUID 1
> +#elif defined __pentium__
> +# define HAS_CPUID 1
> +# define HAS_I586 1
> +# define HAS_I686 0
> +#elif (defined __pentiumpro__ || defined __pentium4__		\
> +       || defined __nocona__ || defined __atom__		\
> +       || defined __core2__ || defined __corei7__		\
> +       || defined __corei7_avx__ || defined __core_avx2__	\
> +       || defined __nehalem__ || defined __sandybridge__	\
> +       || defined __haswell__  || defined __knl__		\
> +       || defined __bonnell__ || defined __silvermont__ 	\
> +       || defined __k6__ || defined __k8__			\
> +       || defined __athlon__ || defined __amdfam10__		\
> +       || defined __bdver1__ || defined __bdver2__		\
> +       || defined __bdver3__ || defined __bdver4__		\
> +       || defined __btver1__ || defined __btver2__)
> +# define HAS_CPUID 1
> +# define HAS_I586 1
> +# define HAS_I686 1
> +#else
> +# define HAS_CPUID 0
> +# define HAS_I586 0
> +# define HAS_I686 0
> +#endif

why is testing for __i686__ & __i586__ unacceptable ?
-mike
H.J. Lu Aug. 18, 2015, 2:56 p.m. UTC | #4 
On Tue, Aug 18, 2015 at 7:32 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 17 Aug 2015 16:10, H.J. Lu wrote:
>> +#ifdef __x86_64__
>> +# define HAS_CPUID 1
>> +#elif defined __pentium__
>> +# define HAS_CPUID 1
>> +# define HAS_I586 1
>> +# define HAS_I686 0
>> +#elif (defined __pentiumpro__ || defined __pentium4__                \
>> +       || defined __nocona__ || defined __atom__             \
>> +       || defined __core2__ || defined __corei7__            \
>> +       || defined __corei7_avx__ || defined __core_avx2__    \
>> +       || defined __nehalem__ || defined __sandybridge__     \
>> +       || defined __haswell__  || defined __knl__            \
>> +       || defined __bonnell__ || defined __silvermont__      \
>> +       || defined __k6__ || defined __k8__                   \
>> +       || defined __athlon__ || defined __amdfam10__         \
>> +       || defined __bdver1__ || defined __bdver2__           \
>> +       || defined __bdver3__ || defined __bdver4__           \
>> +       || defined __btver1__ || defined __btver2__)
>> +# define HAS_CPUID 1
>> +# define HAS_I586 1
>> +# define HAS_I686 1
>> +#else
>> +# define HAS_CPUID 0
>> +# define HAS_I586 0
>> +# define HAS_I686 0
>> +#endif
>
> why is testing for __i686__ & __i586__ unacceptable ?
> -mike

-march=haswell doesn't define __i686__ since __i686__ is
mapped to -mach=i686.
Carlos O'Donell Aug. 18, 2015, 3:49 p.m. UTC | #5 
On 08/18/2015 09:34 AM, H.J. Lu wrote:
> On Mon, Aug 17, 2015 at 10:24 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
>> On Mon, Aug 17, 2015 at 04:10:05PM -0700, H.J. Lu wrote:
>>> cpuid, i586 and i686 instructions are available if the processor
>>> specified by -march= supports them.  We can use this information
>>> to determine whether those instructions can be used safely.
>>>
>>> OK for master?
>>>
>> Patch itself looks ok. It fixes theoretical problem that if there would
>> be new processor without cpuid it would cause problem.
>>
>> But could you explain rationale? Here you replaced one like with
>> equivalent longer macro and HAS_I586 is never used. Do you plan followup
>> patches that use that?
> 
> I will use HAS_I586 and HAS_I686 in multi-arch for i486 and i586
> I am working on.

Does this include folding i486 up a directory and doing away with the
i386 support? Since we need the i486 instructions to implement NPTL.

https://sourceware.org/glibc/wiki/Development_Todo/Master#i386

Cheers,
Carlos.
H.J. Lu Aug. 18, 2015, 3:52 p.m. UTC | #6 
On Tue, Aug 18, 2015 at 8:49 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/18/2015 09:34 AM, H.J. Lu wrote:
>> On Mon, Aug 17, 2015 at 10:24 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
>>> On Mon, Aug 17, 2015 at 04:10:05PM -0700, H.J. Lu wrote:
>>>> cpuid, i586 and i686 instructions are available if the processor
>>>> specified by -march= supports them.  We can use this information
>>>> to determine whether those instructions can be used safely.
>>>>
>>>> OK for master?
>>>>
>>> Patch itself looks ok. It fixes theoretical problem that if there would
>>> be new processor without cpuid it would cause problem.
>>>
>>> But could you explain rationale? Here you replaced one like with
>>> equivalent longer macro and HAS_I586 is never used. Do you plan followup
>>> patches that use that?
>>
>> I will use HAS_I586 and HAS_I686 in multi-arch for i486 and i586
>> I am working on.
>
> Does this include folding i486 up a directory and doing away with the
> i386 support? Since we need the i486 instructions to implement NPTL.
>
> https://sourceware.org/glibc/wiki/Development_Todo/Master#i386
>

I will investigate it and it will take a while.
Carlos O'Donell Aug. 18, 2015, 3:56 p.m. UTC | #7 
On 08/18/2015 11:52 AM, H.J. Lu wrote:
> On Tue, Aug 18, 2015 at 8:49 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 08/18/2015 09:34 AM, H.J. Lu wrote:
>>> On Mon, Aug 17, 2015 at 10:24 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
>>>> On Mon, Aug 17, 2015 at 04:10:05PM -0700, H.J. Lu wrote:
>>>>> cpuid, i586 and i686 instructions are available if the processor
>>>>> specified by -march= supports them.  We can use this information
>>>>> to determine whether those instructions can be used safely.
>>>>>
>>>>> OK for master?
>>>>>
>>>> Patch itself looks ok. It fixes theoretical problem that if there would
>>>> be new processor without cpuid it would cause problem.
>>>>
>>>> But could you explain rationale? Here you replaced one like with
>>>> equivalent longer macro and HAS_I586 is never used. Do you plan followup
>>>> patches that use that?
>>>
>>> I will use HAS_I586 and HAS_I686 in multi-arch for i486 and i586
>>> I am working on.
>>
>> Does this include folding i486 up a directory and doing away with the
>> i386 support? Since we need the i486 instructions to implement NPTL.
>>
>> https://sourceware.org/glibc/wiki/Development_Todo/Master#i386
>>
> 
> I will investigate it and it will take a while.
> 

Thanks :-)

Cheers,
Carlos.
Mike Frysinger Aug. 18, 2015, 4:02 p.m. UTC | #8 
On 18 Aug 2015 07:56, H.J. Lu wrote:
> On Tue, Aug 18, 2015 at 7:32 AM, Mike Frysinger wrote:
> > On 17 Aug 2015 16:10, H.J. Lu wrote:
> >> +#ifdef __x86_64__
> >> +# define HAS_CPUID 1
> >> +#elif defined __pentium__
> >> +# define HAS_CPUID 1
> >> +# define HAS_I586 1
> >> +# define HAS_I686 0
> >> +#elif (defined __pentiumpro__ || defined __pentium4__                \
> >> +       || defined __nocona__ || defined __atom__             \
> >> +       || defined __core2__ || defined __corei7__            \
> >> +       || defined __corei7_avx__ || defined __core_avx2__    \
> >> +       || defined __nehalem__ || defined __sandybridge__     \
> >> +       || defined __haswell__  || defined __knl__            \
> >> +       || defined __bonnell__ || defined __silvermont__      \
> >> +       || defined __k6__ || defined __k8__                   \
> >> +       || defined __athlon__ || defined __amdfam10__         \
> >> +       || defined __bdver1__ || defined __bdver2__           \
> >> +       || defined __bdver3__ || defined __bdver4__           \
> >> +       || defined __btver1__ || defined __btver2__)
> >> +# define HAS_CPUID 1
> >> +# define HAS_I586 1
> >> +# define HAS_I686 1
> >> +#else
> >> +# define HAS_CPUID 0
> >> +# define HAS_I586 0
> >> +# define HAS_I686 0
> >> +#endif
> >
> > why is testing for __i686__ & __i586__ unacceptable ?
> 
> -march=haswell doesn't define __i686__ since __i686__ is
> mapped to -mach=i686.

that's not entirely accurate.  i did check before posting, but it looks like
the first two entries do set up i586 and i686 correctly (and i stopped after
spot checking the first two).  so i guess change __pentium__ to __i586__ and
__pentiumpro__ to __i686__ ?
-mike
H.J. Lu Aug. 18, 2015, 4:08 p.m. UTC | #9 
On Tue, Aug 18, 2015 at 9:02 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 18 Aug 2015 07:56, H.J. Lu wrote:
>> On Tue, Aug 18, 2015 at 7:32 AM, Mike Frysinger wrote:
>> > On 17 Aug 2015 16:10, H.J. Lu wrote:
>> >> +#ifdef __x86_64__
>> >> +# define HAS_CPUID 1
>> >> +#elif defined __pentium__
>> >> +# define HAS_CPUID 1
>> >> +# define HAS_I586 1
>> >> +# define HAS_I686 0
>> >> +#elif (defined __pentiumpro__ || defined __pentium4__                \
>> >> +       || defined __nocona__ || defined __atom__             \
>> >> +       || defined __core2__ || defined __corei7__            \
>> >> +       || defined __corei7_avx__ || defined __core_avx2__    \
>> >> +       || defined __nehalem__ || defined __sandybridge__     \
>> >> +       || defined __haswell__  || defined __knl__            \
>> >> +       || defined __bonnell__ || defined __silvermont__      \
>> >> +       || defined __k6__ || defined __k8__                   \
>> >> +       || defined __athlon__ || defined __amdfam10__         \
>> >> +       || defined __bdver1__ || defined __bdver2__           \
>> >> +       || defined __bdver3__ || defined __bdver4__           \
>> >> +       || defined __btver1__ || defined __btver2__)
>> >> +# define HAS_CPUID 1
>> >> +# define HAS_I586 1
>> >> +# define HAS_I686 1
>> >> +#else
>> >> +# define HAS_CPUID 0
>> >> +# define HAS_I586 0
>> >> +# define HAS_I686 0
>> >> +#endif
>> >
>> > why is testing for __i686__ & __i586__ unacceptable ?
>>
>> -march=haswell doesn't define __i686__ since __i686__ is
>> mapped to -mach=i686.
>
> that's not entirely accurate.  i did check before posting, but it looks like
> the first two entries do set up i586 and i686 correctly (and i stopped after
> spot checking the first two).  so i guess change __pentium__ to __i586__ and
> __pentiumpro__ to __i686__ ?
> -mike

My change is based on sysdeps/x86/bits/string.h and
sysdeps/x86/bits/byteswap.h.  None of them check
__i586__ nor __i686__.
Joseph Myers Aug. 18, 2015, 4:16 p.m. UTC | #10 
On Tue, 18 Aug 2015, H.J. Lu wrote:

> My change is based on sysdeps/x86/bits/string.h and
> sysdeps/x86/bits/byteswap.h.  None of them check
> __i586__ nor __i686__.

Maybe

(a) there should be a new (installed) bits/ header for x86, say 
bits/x86-arch.h, that defines __x86_arch to 4, 5 or 6 depending on such 
macros, so the logic doesn't need duplicating;

(b) the header should test for the probably much smaller number of 486 / 
586 processors supported by GCC, with the default being i686, to avoid 
keeping needing updating it for future processors;

(c) GCC should add such a predefined macro, with bits/x86-arch.h only 
defining it itself if not already defined, as further future-proofing 
against anyone adding new support for an i486-class or i586-class 
processor with new predefined macros?

(Cf. sysdeps/arm/memcpy.S working based on a list of ARM architectures 
without preload support, instead of a list with it; and the __ARM_ARCH 
macro from ACLE in newer versions of GCC.)
H.J. Lu Aug. 18, 2015, 5:30 p.m. UTC | #11 
On Tue, Aug 18, 2015 at 9:16 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Tue, 18 Aug 2015, H.J. Lu wrote:
>
>> My change is based on sysdeps/x86/bits/string.h and
>> sysdeps/x86/bits/byteswap.h.  None of them check
>> __i586__ nor __i686__.
>
> Maybe
>
> (a) there should be a new (installed) bits/ header for x86, say
> bits/x86-arch.h, that defines __x86_arch to 4, 5 or 6 depending on such
> macros, so the logic doesn't need duplicating;
>

We can add cpu-features.h to installed header to provide both
run-time and compile-time CPU feature detection.
Joseph Myers Aug. 18, 2015, 8:21 p.m. UTC | #12 
On Tue, 18 Aug 2015, H.J. Lu wrote:

> On Tue, Aug 18, 2015 at 9:16 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> > On Tue, 18 Aug 2015, H.J. Lu wrote:
> >
> >> My change is based on sysdeps/x86/bits/string.h and
> >> sysdeps/x86/bits/byteswap.h.  None of them check
> >> __i586__ nor __i686__.
> >
> > Maybe
> >
> > (a) there should be a new (installed) bits/ header for x86, say
> > bits/x86-arch.h, that defines __x86_arch to 4, 5 or 6 depending on such
> > macros, so the logic doesn't need duplicating;
> >
> 
> We can add cpu-features.h to installed header to provide both
> run-time and compile-time CPU feature detection.

What I'm suggesting requires a header that does not define or use any 
identifiers outside of the implementation namespace, so that it can be 
included by other installed headers.  This is substantially different from 
cpu-features.h.
Mike Frysinger Aug. 19, 2015, 3:10 a.m. UTC | #13 
On 18 Aug 2015 09:08, H.J. Lu wrote:
> On Tue, Aug 18, 2015 at 9:02 AM, Mike Frysinger wrote:
> > On 18 Aug 2015 07:56, H.J. Lu wrote:
> >> On Tue, Aug 18, 2015 at 7:32 AM, Mike Frysinger wrote:
> >> > On 17 Aug 2015 16:10, H.J. Lu wrote:
> >> >> +#ifdef __x86_64__
> >> >> +# define HAS_CPUID 1
> >> >> +#elif defined __pentium__
> >> >> +# define HAS_CPUID 1
> >> >> +# define HAS_I586 1
> >> >> +# define HAS_I686 0
> >> >> +#elif (defined __pentiumpro__ || defined __pentium4__                \
> >> >> +       || defined __nocona__ || defined __atom__             \
> >> >> +       || defined __core2__ || defined __corei7__            \
> >> >> +       || defined __corei7_avx__ || defined __core_avx2__    \
> >> >> +       || defined __nehalem__ || defined __sandybridge__     \
> >> >> +       || defined __haswell__  || defined __knl__            \
> >> >> +       || defined __bonnell__ || defined __silvermont__      \
> >> >> +       || defined __k6__ || defined __k8__                   \
> >> >> +       || defined __athlon__ || defined __amdfam10__         \
> >> >> +       || defined __bdver1__ || defined __bdver2__           \
> >> >> +       || defined __bdver3__ || defined __bdver4__           \
> >> >> +       || defined __btver1__ || defined __btver2__)
> >> >> +# define HAS_CPUID 1
> >> >> +# define HAS_I586 1
> >> >> +# define HAS_I686 1
> >> >> +#else
> >> >> +# define HAS_CPUID 0
> >> >> +# define HAS_I586 0
> >> >> +# define HAS_I686 0
> >> >> +#endif
> >> >
> >> > why is testing for __i686__ & __i586__ unacceptable ?
> >>
> >> -march=haswell doesn't define __i686__ since __i686__ is
> >> mapped to -mach=i686.
> >
> > that's not entirely accurate.  i did check before posting, but it looks like
> > the first two entries do set up i586 and i686 correctly (and i stopped after
> > spot checking the first two).  so i guess change __pentium__ to __i586__ and
> > __pentiumpro__ to __i686__ ?
> 
> My change is based on sysdeps/x86/bits/string.h and
> sysdeps/x86/bits/byteswap.h.  None of them check
> __i586__ nor __i686__.

i'm aware of the provenance.  that's not a reason to not improve things :).
-mike
diff mbox

Patch

diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index 2d744ac..16d78c5 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -40,7 +40,7 @@  init_cpu_features (struct cpu_features *cpu_features)
   unsigned int model = 0;
   enum cpu_features_kind kind;
 
-#if !defined __i586__ && !defined __i686__ && !defined __x86_64__
+#if !HAS_CPUID
   if (__get_cpuid_max (0, 0) == 0)
     {
       kind = arch_kind_other;
@@ -212,7 +212,7 @@  init_cpu_features (struct cpu_features *cpu_features)
 	}
     }
 
-#if !defined __i586__ && !defined __i686__ && !defined __x86_64__
+#if !HAS_CPUID
 no_cpuid:
 #endif
 
diff --git a/sysdeps/x86/cpu-features.h b/sysdeps/x86/cpu-features.h
index fce30c3..795d653 100644
--- a/sysdeps/x86/cpu-features.h
+++ b/sysdeps/x86/cpu-features.h
@@ -251,4 +251,31 @@  extern const struct cpu_features *__get_cpu_features (void)
 
 #endif	/* !__ASSEMBLER__ */
 
+#ifdef __x86_64__
+# define HAS_CPUID 1
+#elif defined __pentium__
+# define HAS_CPUID 1
+# define HAS_I586 1
+# define HAS_I686 0
+#elif (defined __pentiumpro__ || defined __pentium4__		\
+       || defined __nocona__ || defined __atom__		\
+       || defined __core2__ || defined __corei7__		\
+       || defined __corei7_avx__ || defined __core_avx2__	\
+       || defined __nehalem__ || defined __sandybridge__	\
+       || defined __haswell__  || defined __knl__		\
+       || defined __bonnell__ || defined __silvermont__ 	\
+       || defined __k6__ || defined __k8__			\
+       || defined __athlon__ || defined __amdfam10__		\
+       || defined __bdver1__ || defined __bdver2__		\
+       || defined __bdver3__ || defined __bdver4__		\
+       || defined __btver1__ || defined __btver2__)
+# define HAS_CPUID 1
+# define HAS_I586 1
+# define HAS_I686 1
+#else
+# define HAS_CPUID 0
+# define HAS_I586 0
+# define HAS_I686 0
+#endif
+
 #endif  /* cpu_features_h */