i386: Call get_available_features for all CPUs with max_level >= 1 [PR100758]

Message ID Y+TjBOP6PzjLP4Ua@tucnak
State Unresolved
Headers
Series i386: Call get_available_features for all CPUs with max_level >= 1 [PR100758] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Feb. 9, 2023, 12:11 p.m. UTC
  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

H.J. Lu Feb. 9, 2023, 3:30 p.m. UTC | #1
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
>
  
Jakub Jelinek Feb. 9, 2023, 3:43 p.m. UTC | #2
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
  
Uros Bizjak Feb. 9, 2023, 4:34 p.m. UTC | #3
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
>
  

Patch

--- 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)