diff mbox series

[X86] Workaround possible CPUID bug in Sandy Bridge.

Message ID 20230808075600.1878105-1-hongtao.liu@intel.com
State New
Headers show
Series [X86] Workaround possible CPUID bug in Sandy Bridge. | expand

Commit Message

liuhongt Aug. 8, 2023, 7:56 a.m. UTC
Don't access leaf 7 subleaf 1 unless subleaf 0 says it is
supported via EAX.

Intel documentation says invalid subleaves return 0. We had been
relying on that behavior instead of checking the max sublef number.

It appears that some Sandy Bridge CPUs return at least the subleaf 0
EDX value for subleaf 1. Best guess is that this is a bug in a
microcode patch since all of the bits we're seeing set in EDX were
introduced after Sandy Bridge was originally released.

This is causing avxvnniint16 to be incorrectly enabled with
-march=native on these CPUs.

BTW: Thanks for reminder from llvm forks Phoebe and Craig.


Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
Ok for trunk and backport?

gcc/ChangeLog:

	* common/config/i386/cpuinfo.h (get_available_features): Check
	EAX for valid subleaf before use CPUID.
---
 gcc/common/config/i386/cpuinfo.h | 84 +++++++++++++++++---------------
 1 file changed, 46 insertions(+), 38 deletions(-)

Comments

Jakub Jelinek Aug. 8, 2023, 8:02 a.m. UTC | #1
On Tue, Aug 08, 2023 at 03:56:00PM +0800, liuhongt via Gcc-patches wrote:
> +      /* According to document, when subleaf is invliad, EAX,EBX,ECX,EDX should

s/invliad/invalid/

	Jakub
Uros Bizjak Aug. 8, 2023, 12:41 p.m. UTC | #2
On Tue, Aug 8, 2023 at 9:58 AM liuhongt <hongtao.liu@intel.com> wrote:
>
> Don't access leaf 7 subleaf 1 unless subleaf 0 says it is
> supported via EAX.
>
> Intel documentation says invalid subleaves return 0. We had been
> relying on that behavior instead of checking the max sublef number.
>
> It appears that some Sandy Bridge CPUs return at least the subleaf 0
> EDX value for subleaf 1. Best guess is that this is a bug in a
> microcode patch since all of the bits we're seeing set in EDX were
> introduced after Sandy Bridge was originally released.
>
> This is causing avxvnniint16 to be incorrectly enabled with
> -march=native on these CPUs.
>
> BTW: Thanks for reminder from llvm forks Phoebe and Craig.
>
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.

Please rather do it in a more self-descriptive way, as proposed in the
attached patch. You won't need a comment then.

(Please note that indentation is wrong in the patch in order to better
see the changes).

Uros.
diff --git a/gcc/common/config/i386/cpuinfo.h b/gcc/common/config/i386/cpuinfo.h
index 30ef0d334ca..49724d2cba1 100644
--- a/gcc/common/config/i386/cpuinfo.h
+++ b/gcc/common/config/i386/cpuinfo.h
@@ -762,7 +762,9 @@ get_available_features (struct __processor_model *cpu_model,
   /* Get Advanced Features at level 7 (eax = 7, ecx = 0/1). */
   if (max_cpuid_level >= 7)
     {
-      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+      unsigned subleaf_level;
+
+      __cpuid_count (7, 0, subleaf_level, ebx, ecx, edx);
       if (ebx & bit_BMI)
 	set_feature (FEATURE_BMI);
       if (ebx & bit_SGX)
@@ -873,8 +875,9 @@ get_available_features (struct __processor_model *cpu_model,
 	  if (edx & bit_AVX512FP16)
 	    set_feature (FEATURE_AVX512FP16);
 	}
-
-      __cpuid_count (7, 1, eax, ebx, ecx, edx);
+      if (subleaf_level >= 1)
+	{
+	  __cpuid_count (7, 1, eax, ebx, ecx, edx);
       if (eax & bit_HRESET)
 	set_feature (FEATURE_HRESET);
       if (eax & bit_CMPCCXADD)
@@ -914,6 +917,7 @@ get_available_features (struct __processor_model *cpu_model,
 	  if (edx & bit_AMX_COMPLEX)
 	    set_feature (FEATURE_AMX_COMPLEX);
 	}
+	}
     }
 
   /* Get Advanced Features at level 0xd (eax = 0xd, ecx = 1). */
diff mbox series

Patch

diff --git a/gcc/common/config/i386/cpuinfo.h b/gcc/common/config/i386/cpuinfo.h
index 30ef0d334ca..24ab2252eb0 100644
--- a/gcc/common/config/i386/cpuinfo.h
+++ b/gcc/common/config/i386/cpuinfo.h
@@ -874,45 +874,53 @@  get_available_features (struct __processor_model *cpu_model,
 	    set_feature (FEATURE_AVX512FP16);
 	}
 
-      __cpuid_count (7, 1, eax, ebx, ecx, edx);
-      if (eax & bit_HRESET)
-	set_feature (FEATURE_HRESET);
-      if (eax & bit_CMPCCXADD)
-	set_feature(FEATURE_CMPCCXADD);
-      if (edx & bit_PREFETCHI)
-	set_feature (FEATURE_PREFETCHI);
-      if (eax & bit_RAOINT)
-	set_feature (FEATURE_RAOINT);
-      if (avx_usable)
-	{
-	  if (eax & bit_AVXVNNI)
-	    set_feature (FEATURE_AVXVNNI);
-	  if (eax & bit_AVXIFMA)
-	    set_feature (FEATURE_AVXIFMA);
-	  if (edx & bit_AVXVNNIINT8)
-	    set_feature (FEATURE_AVXVNNIINT8);
-	  if (edx & bit_AVXNECONVERT)
-	    set_feature (FEATURE_AVXNECONVERT);
-	  if (edx & bit_AVXVNNIINT16)
-	    set_feature (FEATURE_AVXVNNIINT16);
-	  if (eax & bit_SM3)
-	    set_feature (FEATURE_SM3);
-	  if (eax & bit_SHA512)
-	    set_feature (FEATURE_SHA512);
-	  if (eax & bit_SM4)
-	    set_feature (FEATURE_SM4);
-	}
-      if (avx512_usable)
-	{
-	  if (eax & bit_AVX512BF16)
-	    set_feature (FEATURE_AVX512BF16);
-	}
-      if (amx_usable)
+      /* According to document, when subleaf is invliad, EAX,EBX,ECX,EDX should
+	 return 0 for CPUID (7, 1, EAX, EBX, ECX, EDX).
+	 But looks like it doesn't satisfy the document on some CPU, refer to
+	 https://reviews.llvm.org/D155145.
+	 Manually check valid subleaf here.  */
+      if (eax)
 	{
-	  if (eax & bit_AMX_FP16)
-	    set_feature (FEATURE_AMX_FP16);
-	  if (edx & bit_AMX_COMPLEX)
-	    set_feature (FEATURE_AMX_COMPLEX);
+	  __cpuid_count (7, 1, eax, ebx, ecx, edx);
+	  if (eax & bit_HRESET)
+	    set_feature (FEATURE_HRESET);
+	  if (eax & bit_CMPCCXADD)
+	    set_feature(FEATURE_CMPCCXADD);
+	  if (edx & bit_PREFETCHI)
+	    set_feature (FEATURE_PREFETCHI);
+	  if (eax & bit_RAOINT)
+	    set_feature (FEATURE_RAOINT);
+	  if (avx_usable)
+	    {
+	      if (eax & bit_AVXVNNI)
+		set_feature (FEATURE_AVXVNNI);
+	      if (eax & bit_AVXIFMA)
+		set_feature (FEATURE_AVXIFMA);
+	      if (edx & bit_AVXVNNIINT8)
+		set_feature (FEATURE_AVXVNNIINT8);
+	      if (edx & bit_AVXNECONVERT)
+		set_feature (FEATURE_AVXNECONVERT);
+	      if (edx & bit_AVXVNNIINT16)
+		set_feature (FEATURE_AVXVNNIINT16);
+	      if (eax & bit_SM3)
+		set_feature (FEATURE_SM3);
+	      if (eax & bit_SHA512)
+		set_feature (FEATURE_SHA512);
+	      if (eax & bit_SM4)
+		set_feature (FEATURE_SM4);
+	    }
+	  if (avx512_usable)
+	    {
+	      if (eax & bit_AVX512BF16)
+		set_feature (FEATURE_AVX512BF16);
+	    }
+	  if (amx_usable)
+	    {
+	      if (eax & bit_AMX_FP16)
+		set_feature (FEATURE_AMX_FP16);
+	      if (edx & bit_AMX_COMPLEX)
+		set_feature (FEATURE_AMX_COMPLEX);
+	    }
 	}
     }