i386: Call get_available_features for all CPUs with max_level >= 1 [PR100758]
Checks
Commit Message
Hi!
get_available_features doesn't depend on cpu_model2->__cpu_{family,model}
and just sets stuff up based on CPUID leaf 1, or some extended ones,
so I wonder why are we calling it separately for Intel, AMD and Zhaoxin
and not for all other CPUs too? I think various programs in the wild
which aren't using __builtin_cpu_{is,supports} just check the various CPUID
leafs and query bits in there, without blacklisting unknown CPU vendors,
so I think even __builtin_cpu_supports ("sse2") etc. should be reliable
if those VENDOR_{CENTAUR,CYRIX,NSC,OTHER} CPUs set those bits in CPUID leaf
1 or some extended ones. Calling it for all CPUs also means it can be
inlined because there will be just a single caller.
I will test on Intel but can't test it on non-Intel (or with some extra
effort on AMD; for both of those arches it should be really no change in
behavior).
Thoughts on this?
2023-02-09 Jakub Jelinek <jakub@redhat.com>
PR target/100758
* common/config/i386/cpuinfo.h (get_zhaoxin_cpu): Formatting fixes.
(cpu_indicator_init): Call get_available_features for all CPUs with
max_level >= 1, rather than just Intel, AMD or Zhaoxin. Formatting
fixes.
Jakub
Comments
On Thu, Feb 9, 2023 at 4:12 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> get_available_features doesn't depend on cpu_model2->__cpu_{family,model}
> and just sets stuff up based on CPUID leaf 1, or some extended ones,
> so I wonder why are we calling it separately for Intel, AMD and Zhaoxin
> and not for all other CPUs too? I think various programs in the wild
> which aren't using __builtin_cpu_{is,supports} just check the various CPUID
> leafs and query bits in there, without blacklisting unknown CPU vendors,
> so I think even __builtin_cpu_supports ("sse2") etc. should be reliable
> if those VENDOR_{CENTAUR,CYRIX,NSC,OTHER} CPUs set those bits in CPUID leaf
> 1 or some extended ones. Calling it for all CPUs also means it can be
> inlined because there will be just a single caller.
>
> I will test on Intel but can't test it on non-Intel (or with some extra
> effort on AMD; for both of those arches it should be really no change in
> behavior).
>
> Thoughts on this?
No objection here. It just isn't easy to verify CPUID behavior on
other processors.
Thanks.
> 2023-02-09 Jakub Jelinek <jakub@redhat.com>
>
> PR target/100758
> * common/config/i386/cpuinfo.h (get_zhaoxin_cpu): Formatting fixes.
> (cpu_indicator_init): Call get_available_features for all CPUs with
> max_level >= 1, rather than just Intel, AMD or Zhaoxin. Formatting
> fixes.
>
> --- gcc/common/config/i386/cpuinfo.h.jj 2023-01-16 11:52:15.910736614 +0100
> +++ gcc/common/config/i386/cpuinfo.h 2023-02-09 12:51:23.539470140 +0100
> @@ -601,8 +601,8 @@ get_intel_cpu (struct __processor_model
>
> static inline const char *
> get_zhaoxin_cpu (struct __processor_model *cpu_model,
> - struct __processor_model2 *cpu_model2,
> - unsigned int *cpu_features2)
> + struct __processor_model2 *cpu_model2,
> + unsigned int *cpu_features2)
> {
> const char *cpu = NULL;
> unsigned int family = cpu_model2->__cpu_family;
> @@ -1016,6 +1016,10 @@ cpu_indicator_init (struct __processor_m
> extended_model = (eax >> 12) & 0xf0;
> extended_family = (eax >> 20) & 0xff;
>
> + /* Find available features. */
> + get_available_features (cpu_model, cpu_model2, cpu_features2,
> + ecx, edx);
> +
> if (vendor == signature_INTEL_ebx)
> {
> /* Adjust model and family for Intel CPUS. */
> @@ -1030,9 +1034,6 @@ cpu_indicator_init (struct __processor_m
> cpu_model2->__cpu_family = family;
> cpu_model2->__cpu_model = model;
>
> - /* Find available features. */
> - get_available_features (cpu_model, cpu_model2, cpu_features2,
> - ecx, edx);
> /* Get CPU type. */
> get_intel_cpu (cpu_model, cpu_model2, cpu_features2);
> cpu_model->__cpu_vendor = VENDOR_INTEL;
> @@ -1049,9 +1050,6 @@ cpu_indicator_init (struct __processor_m
> cpu_model2->__cpu_family = family;
> cpu_model2->__cpu_model = model;
>
> - /* Find available features. */
> - get_available_features (cpu_model, cpu_model2, cpu_features2,
> - ecx, edx);
> /* Get CPU type. */
> get_amd_cpu (cpu_model, cpu_model2, cpu_features2);
> cpu_model->__cpu_vendor = VENDOR_AMD;
> @@ -1059,22 +1057,17 @@ cpu_indicator_init (struct __processor_m
> else if (vendor == signature_CENTAUR_ebx && family < 0x07)
> cpu_model->__cpu_vendor = VENDOR_CENTAUR;
> else if (vendor == signature_SHANGHAI_ebx
> - || vendor == signature_CENTAUR_ebx)
> + || vendor == signature_CENTAUR_ebx)
> {
> /* Adjust model and family for ZHAOXIN CPUS. */
> if (family == 0x07)
> - {
> - model += extended_model;
> - }
> + model += extended_model;
>
> cpu_model2->__cpu_family = family;
> cpu_model2->__cpu_model = model;
>
> - /* Find available features. */
> - get_available_features (cpu_model, cpu_model2, cpu_features2,
> - ecx, edx);
> /* Get CPU type. */
> - get_zhaoxin_cpu (cpu_model, cpu_model2,cpu_features2);
> + get_zhaoxin_cpu (cpu_model, cpu_model2, cpu_features2);
> cpu_model->__cpu_vendor = VENDOR_ZHAOXIN;
> }
> else if (vendor == signature_CYRIX_ebx)
>
> Jakub
>
On Thu, Feb 09, 2023 at 07:30:52AM -0800, H.J. Lu wrote:
> On Thu, Feb 9, 2023 at 4:12 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > get_available_features doesn't depend on cpu_model2->__cpu_{family,model}
> > and just sets stuff up based on CPUID leaf 1, or some extended ones,
> > so I wonder why are we calling it separately for Intel, AMD and Zhaoxin
> > and not for all other CPUs too? I think various programs in the wild
> > which aren't using __builtin_cpu_{is,supports} just check the various CPUID
> > leafs and query bits in there, without blacklisting unknown CPU vendors,
> > so I think even __builtin_cpu_supports ("sse2") etc. should be reliable
> > if those VENDOR_{CENTAUR,CYRIX,NSC,OTHER} CPUs set those bits in CPUID leaf
> > 1 or some extended ones. Calling it for all CPUs also means it can be
> > inlined because there will be just a single caller.
> >
> > I will test on Intel but can't test it on non-Intel (or with some extra
> > effort on AMD; for both of those arches it should be really no change in
> > behavior).
> >
> > Thoughts on this?
>
> No objection here. It just isn't easy to verify CPUID behavior on
> other processors.
Sure, worst case it can be reverted or exceptions could be added if some
CPUs misbehave but then we'd hopefully have detailed into on how exactly it
behaves.
FYI, I've successfully bootstrapped/regtested this on Intel i9-7960X
and Martin Liska has regtested it with just i386.exp tests on AMD.
Uros, is this ok now?
> > 2023-02-09 Jakub Jelinek <jakub@redhat.com>
> >
> > PR target/100758
> > * common/config/i386/cpuinfo.h (get_zhaoxin_cpu): Formatting fixes.
> > (cpu_indicator_init): Call get_available_features for all CPUs with
> > max_level >= 1, rather than just Intel, AMD or Zhaoxin. Formatting
> > fixes.
Jakub
On Thu, Feb 9, 2023 at 4:43 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Feb 09, 2023 at 07:30:52AM -0800, H.J. Lu wrote:
> > On Thu, Feb 9, 2023 at 4:12 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > > get_available_features doesn't depend on cpu_model2->__cpu_{family,model}
> > > and just sets stuff up based on CPUID leaf 1, or some extended ones,
> > > so I wonder why are we calling it separately for Intel, AMD and Zhaoxin
> > > and not for all other CPUs too? I think various programs in the wild
> > > which aren't using __builtin_cpu_{is,supports} just check the various CPUID
> > > leafs and query bits in there, without blacklisting unknown CPU vendors,
> > > so I think even __builtin_cpu_supports ("sse2") etc. should be reliable
> > > if those VENDOR_{CENTAUR,CYRIX,NSC,OTHER} CPUs set those bits in CPUID leaf
> > > 1 or some extended ones. Calling it for all CPUs also means it can be
> > > inlined because there will be just a single caller.
> > >
> > > I will test on Intel but can't test it on non-Intel (or with some extra
> > > effort on AMD; for both of those arches it should be really no change in
> > > behavior).
> > >
> > > Thoughts on this?
> >
> > No objection here. It just isn't easy to verify CPUID behavior on
> > other processors.
>
> Sure, worst case it can be reverted or exceptions could be added if some
> CPUs misbehave but then we'd hopefully have detailed into on how exactly it
> behaves.
>
> FYI, I've successfully bootstrapped/regtested this on Intel i9-7960X
> and Martin Liska has regtested it with just i386.exp tests on AMD.
>
> Uros, is this ok now?
OK. Let's go forward with the patch.
Thanks,
Uros.
>
> > > 2023-02-09 Jakub Jelinek <jakub@redhat.com>
> > >
> > > PR target/100758
> > > * common/config/i386/cpuinfo.h (get_zhaoxin_cpu): Formatting fixes.
> > > (cpu_indicator_init): Call get_available_features for all CPUs with
> > > max_level >= 1, rather than just Intel, AMD or Zhaoxin. Formatting
> > > fixes.
>
> Jakub
>
@@ -601,8 +601,8 @@ get_intel_cpu (struct __processor_model
static inline const char *
get_zhaoxin_cpu (struct __processor_model *cpu_model,
- struct __processor_model2 *cpu_model2,
- unsigned int *cpu_features2)
+ struct __processor_model2 *cpu_model2,
+ unsigned int *cpu_features2)
{
const char *cpu = NULL;
unsigned int family = cpu_model2->__cpu_family;
@@ -1016,6 +1016,10 @@ cpu_indicator_init (struct __processor_m
extended_model = (eax >> 12) & 0xf0;
extended_family = (eax >> 20) & 0xff;
+ /* Find available features. */
+ get_available_features (cpu_model, cpu_model2, cpu_features2,
+ ecx, edx);
+
if (vendor == signature_INTEL_ebx)
{
/* Adjust model and family for Intel CPUS. */
@@ -1030,9 +1034,6 @@ cpu_indicator_init (struct __processor_m
cpu_model2->__cpu_family = family;
cpu_model2->__cpu_model = model;
- /* Find available features. */
- get_available_features (cpu_model, cpu_model2, cpu_features2,
- ecx, edx);
/* Get CPU type. */
get_intel_cpu (cpu_model, cpu_model2, cpu_features2);
cpu_model->__cpu_vendor = VENDOR_INTEL;
@@ -1049,9 +1050,6 @@ cpu_indicator_init (struct __processor_m
cpu_model2->__cpu_family = family;
cpu_model2->__cpu_model = model;
- /* Find available features. */
- get_available_features (cpu_model, cpu_model2, cpu_features2,
- ecx, edx);
/* Get CPU type. */
get_amd_cpu (cpu_model, cpu_model2, cpu_features2);
cpu_model->__cpu_vendor = VENDOR_AMD;
@@ -1059,22 +1057,17 @@ cpu_indicator_init (struct __processor_m
else if (vendor == signature_CENTAUR_ebx && family < 0x07)
cpu_model->__cpu_vendor = VENDOR_CENTAUR;
else if (vendor == signature_SHANGHAI_ebx
- || vendor == signature_CENTAUR_ebx)
+ || vendor == signature_CENTAUR_ebx)
{
/* Adjust model and family for ZHAOXIN CPUS. */
if (family == 0x07)
- {
- model += extended_model;
- }
+ model += extended_model;
cpu_model2->__cpu_family = family;
cpu_model2->__cpu_model = model;
- /* Find available features. */
- get_available_features (cpu_model, cpu_model2, cpu_features2,
- ecx, edx);
/* Get CPU type. */
- get_zhaoxin_cpu (cpu_model, cpu_model2,cpu_features2);
+ get_zhaoxin_cpu (cpu_model, cpu_model2, cpu_features2);
cpu_model->__cpu_vendor = VENDOR_ZHAOXIN;
}
else if (vendor == signature_CYRIX_ebx)