[PATCH, i386] Handle extended family cpuid info for AMD

[PATCH, i386] Handle extended family cpuid info for AMD

[Gopalasubramanian, Ganesh]

Hi,

The below patch handles the AMD's cpuid family information.
With the information from cpuid, BTVER2 cpu for -march=native flag is handled.

Bootstrap passes.
Is it OK for trunk and branches?

Regards
Ganesh 

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 6223bd6..3f8bb2c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-07-31  Ganesh Gopalasubramanian  <Ganesh.Gopalasubramanian@amd.com>
+
+	* tree-sra.c (host_detect_local_cpu): Handle AMD's extended family
+	information. Handle BTVER2 cpu with cpuid family value.  
+
 2014-07-30  Martin Jambor  <mjambor@suse.cz>
 
 	* tree-sra.c (sra_ipa_modify_assign): Change type of the first
diff --git a/gcc/config/i386/driver-i386.c b/gcc/config/i386/driver-i386.c
index 1c6385f..21ae1f3 100644
--- a/gcc/config/i386/driver-i386.c
+++ b/gcc/config/i386/driver-i386.c
@@ -432,7 +432,8 @@ const char *host_detect_local_cpu (int argc, const char **argv)
 
   model = (eax >> 4) & 0x0f;
   family = (eax >> 8) & 0x0f;
-  if (vendor == signature_INTEL_ebx)
+  if ((vendor == signature_INTEL_ebx) ||
+      (vendor == signature_AMD_ebx))
     {
       unsigned int extended_model, extended_family;
 
@@ -576,7 +577,7 @@ const char *host_detect_local_cpu (int argc, const char **argv)
 
       if (name == signature_NSC_ebx)
 	processor = PROCESSOR_GEODE;
-      else if (has_movbe)
+      else if (has_movbe && family == 22)
 	processor = PROCESSOR_BTVER2;
       else if (has_avx2)
         processor = PROCESSOR_BDVER4;

Re: [PATCH, i386] Handle extended family cpuid info for AMD

[Uros Bizjak]

On Thu, Jul 31, 2014 at 1:28 PM, Gopalasubramanian, Ganesh
<Ganesh.Gopalasubramanian@amd.com> wrote:

> The below patch handles the AMD's cpuid family information.
> With the information from cpuid, BTVER2 cpu for -march=native flag is handled.

But, looking to processor_alias_table in config/i386/i386.c, only
PROCESSOR_BTVER2 defines PTA_MOVBE. According to this, the logic is
already correct, so the patch is not needed.

Uros.

RE: [PATCH, i386] Handle extended family cpuid info for AMD

[Gopalasubramanian, Ganesh]

> But, looking to processor_alias_table in config/i386/i386.c, only
> PROCESSOR_BTVER2 defines PTA_MOVBE. According to this, the logic is already correct, so the patch is not needed.

We are evaluating bdver4 cpu. Bdver4 also supports MOVBE. I will submit patch for bdver4 PTA after our evaluation.

Ganesh.

Re: [PATCH, i386] Handle extended family cpuid info for AMD

[Uros Bizjak]

On Thu, Jul 31, 2014 at 1:46 PM, Gopalasubramanian, Ganesh
<Ganesh.Gopalasubramanian@amd.com> wrote:
>> But, looking to processor_alias_table in config/i386/i386.c, only
>> PROCESSOR_BTVER2 defines PTA_MOVBE. According to this, the logic is already correct, so the patch is not needed.
>
> We are evaluating bdver4 cpu. Bdver4 also supports MOVBE. I will submit patch for bdver4 PTA after our evaluation.

Then just use:

--cut here--
Index: driver-i386.c
===================================================================
--- driver-i386.c       (revision 213342)
+++ driver-i386.c       (working copy)
@@ -576,10 +576,10 @@ const char *host_detect_local_cpu (int argc, const

       if (name == signature_NSC_ebx)
        processor = PROCESSOR_GEODE;
+      else if (has_avx2)
+        processor = PROCESSOR_BDVER4;
       else if (has_movbe)
        processor = PROCESSOR_BTVER2;
-      else if (has_avx2)
-        processor = PROCESSOR_BDVER4;
       else if (has_xsaveopt)
         processor = PROCESSOR_BDVER3;
       else if (has_bmi)
--cut here--

Uros.

RE: [PATCH, i386] Handle extended family cpuid info for AMD

[Gopalasubramanian, Ganesh]

> Then just use:

> +      else if (has_avx2)
> +        processor = PROCESSOR_BDVER4;
>       else if (has_movbe)
>        processor = PROCESSOR_BTVER2;
>-      else if (has_avx2)
>-        processor = PROCESSOR_BDVER4;
>      else if (has_xsaveopt)

In that case, with earlier GCC versions where we don’t have bdver4 added, the fall back would be BTVER2, 
whereas a BD variant is more desirable.

Ganesh

Re: [PATCH, i386] Handle extended family cpuid info for AMD

[Uros Bizjak]

On Thu, Jul 31, 2014 at 1:54 PM, Gopalasubramanian, Ganesh
<Ganesh.Gopalasubramanian@amd.com> wrote:
>> Then just use:
>
>> +      else if (has_avx2)
>> +        processor = PROCESSOR_BDVER4;
>>       else if (has_movbe)
>>        processor = PROCESSOR_BTVER2;
>>-      else if (has_avx2)
>>-        processor = PROCESSOR_BDVER4;
>>      else if (has_xsaveopt)
>
> In that case, with earlier GCC versions where we don’t have bdver4 added, the fall back would be BTVER2,
> whereas a BD variant is more desirable.

I would like to have a check for a family at the beginning, something like:

      if (name == signature_NSC_ebx)
    processor = PROCESSOR_GEODE;
      else if (family == 22)
    {
      if (has_movbe)
        processor = PROCESSOR_BTVER2;
      else
        processor = PROCESSOR_BTVER1;
    }
      else if (has_avx2)
        processor = PROCESSOR_BDVER4;
      else if (has_xsaveopt)

Please also fix ChangeLog entry.

Uros.

RE: [PATCH, i386] Handle extended family cpuid info for AMD

[Gopalasubramanian, Ganesh]

Uros!

> I would like to have a check for a family at the beginning, something like:

>      if (name == signature_NSC_ebx)
>    processor = PROCESSOR_GEODE;
>      else if (family == 22)
>    {
>      if (has_movbe)

I get your idea of having the family checked first and then differentiating with cpuid info later.
But, this case is getting interesting because, BTVER1 and BTVER2 are two variants but don't really have same family numbers.
BTVER1 is family 14h and BTVER2 is family 16h. I don't see near term plans for any additional cpus to either 14h or 16h.
Given that fact, this particular check is applicable only for BTVER2.
In that case, having 

      else if (family == 22)
        if (has_movbe)
           processor = PROCESSOR_BTVER2;

looks odd. 

Regards
Ganesh

Re: [PATCH, i386] Handle extended family cpuid info for AMD

[Uros Bizjak]

On Fri, Aug 1, 2014 at 8:18 AM, Gopalasubramanian, Ganesh
<Ganesh.Gopalasubramanian@amd.com> wrote:

>> I would like to have a check for a family at the beginning, something like:
>
>>      if (name == signature_NSC_ebx)
>>    processor = PROCESSOR_GEODE;
>>      else if (family == 22)
>>    {
>>      if (has_movbe)
>
> I get your idea of having the family checked first and then differentiating with cpuid info later.
> But, this case is getting interesting because, BTVER1 and BTVER2 are two variants but don't really have same family numbers.
> BTVER1 is family 14h and BTVER2 is family 16h. I don't see near term plans for any additional cpus to either 14h or 16h.
> Given that fact, this particular check is applicable only for BTVER2.
> In that case, having
>
>       else if (family == 22)
>         if (has_movbe)
>            processor = PROCESSOR_BTVER2;
>
> looks odd.

In this case, having only check for family ID should be enough. If
BTVER1 and BTVER2 can be uniquely determined by their family IDs ,
then:

... (signature checks)
else if (family == 0x16h)
  processor = PROCESSOR_BTVER2;
else if (family == 0x14h)
  processor = PROCESSOR_BTVER1;
else
  ... (detect processor in a generic way by its cpuid features.)

IMO, this would be the most future-proof approach. Signature checks
will override family id checks which will override cpuid checks.

Uros.

RE: [PATCH, i386] Handle extended family cpuid info for AMD

[Gopalasubramanian, Ganesh]

> In this case, having only check for family ID should be enough. If
>BTVER1 and BTVER2 can be uniquely determined by their family IDs ,

>IMO, this would be the most future-proof approach. Signature checks will override family id checks which will override cpuid checks.

Thank you Uros!

I have modified source only for BTVER2. 
The way BTVER1 is currently assigned to processor includes more than one family. So, I am leaving that unmoved.

Bootstrap passes.
Is it OK for trunk and backport to open branches.

Regards
-Ganesh


diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 706fedc..202bd99 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-08-01  Ganesh Gopalasubramanian  <Ganesh.Gopalasubramanian@amd.com>
+
+	* config/i386/driver-i386.c (host_detect_local_cpu): Handle AMD's extended family
+	information. Handle BTVER2 cpu with cpuid family value.  
+
 2014-07-31  James Greenhalgh  <james.greenhalgh@arm.com>
 
 	* config/aarch64/arm_neon.h (vpadd_<suf><8,16,32,64>): Move to
diff --git a/gcc/config/i386/driver-i386.c b/gcc/config/i386/driver-i386.c
index 1c6385f..0402c90 100644
--- a/gcc/config/i386/driver-i386.c
+++ b/gcc/config/i386/driver-i386.c
@@ -432,7 +432,8 @@ const char *host_detect_local_cpu (int argc, const char **argv)
 
   model = (eax >> 4) & 0x0f;
   family = (eax >> 8) & 0x0f;
-  if (vendor == signature_INTEL_ebx)
+  if ((vendor == signature_INTEL_ebx) ||
+      (vendor == signature_AMD_ebx))
     {
       unsigned int extended_model, extended_family;
 
@@ -576,7 +577,7 @@ const char *host_detect_local_cpu (int argc, const char **argv)
 
       if (name == signature_NSC_ebx)
 	processor = PROCESSOR_GEODE;
-      else if (has_movbe)
+      else if (family == 22)
 	processor = PROCESSOR_BTVER2;
       else if (has_avx2)
         processor = PROCESSOR_BDVER4;

Re: [PATCH, i386] Handle extended family cpuid info for AMD

[Jakub Jelinek]

On Fri, Aug 01, 2014 at 02:52:28PM +0000, Gopalasubramanian, Ganesh wrote:
> --- a/gcc/config/i386/driver-i386.c
> +++ b/gcc/config/i386/driver-i386.c
> @@ -432,7 +432,8 @@ const char *host_detect_local_cpu (int argc, const char **argv)
>  
>    model = (eax >> 4) & 0x0f;
>    family = (eax >> 8) & 0x0f;
> -  if (vendor == signature_INTEL_ebx)
> +  if ((vendor == signature_INTEL_ebx) ||
> +      (vendor == signature_AMD_ebx))

Wrong formatting.  No ()s around the comparisons needed, and
|| should go on the second line, not first.

> @@ -576,7 +577,7 @@ const char *host_detect_local_cpu (int argc, const char **argv)
>  
>        if (name == signature_NSC_ebx)
>  	processor = PROCESSOR_GEODE;
> -      else if (has_movbe)
> +      else if (family == 22)
>  	processor = PROCESSOR_BTVER2;

Wouldn't it be safer to use has_movbe && family == 22?
I mean, especially with emulators which choose to provide some architecture,
but disable some CPUID flags it is IMHO safer to also check the flags.

	Jakub

Re: [PATCH, i386] Handle extended family cpuid info for AMD

[Uros Bizjak]

On Fri, Aug 1, 2014 at 5:00 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> --- a/gcc/config/i386/driver-i386.c
>> +++ b/gcc/config/i386/driver-i386.c
>> @@ -432,7 +432,8 @@ const char *host_detect_local_cpu (int argc, const char **argv)
>>
>>    model = (eax >> 4) & 0x0f;
>>    family = (eax >> 8) & 0x0f;
>> -  if (vendor == signature_INTEL_ebx)
>> +  if ((vendor == signature_INTEL_ebx) ||
>> +      (vendor == signature_AMD_ebx))
>
> Wrong formatting.  No ()s around the comparisons needed, and
> || should go on the second line, not first.
>
>> @@ -576,7 +577,7 @@ const char *host_detect_local_cpu (int argc, const char **argv)
>>
>>        if (name == signature_NSC_ebx)
>>       processor = PROCESSOR_GEODE;
>> -      else if (has_movbe)
>> +      else if (family == 22)
>>       processor = PROCESSOR_BTVER2;
>
> Wouldn't it be safer to use has_movbe && family == 22?
> I mean, especially with emulators which choose to provide some architecture,
> but disable some CPUID flags it is IMHO safer to also check the flags.

OK, after rethinking this, let's go with the safer original patch
(sorry for a bit of confusion, but we cleared a lot of open questions
during the discussion).

The original patch is OK for mainline and branches.

OTOH, it looks we will have to redesign -march=native someday to only
pass options, derived from cpuid and eventually pass -mtune based on
detected family.

Thanks,
Uros.