diff mbox series

[1/2] x86: correct ISA-used version recording

Message ID 36de7adb-bc86-4d65-9a75-4cae7c426b92@suse.com
State New
Headers
Series x86: ISA-used version recording |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jan Beulich Jan. 21, 2025, 1:39 p.m. UTC 
  Updating should be based solely on the current instruction. For example,
recording of VEX-encoded insns as v3 should be independent of there
being earlier AMX insns.

Further for BASELINE only a very limited set of the
GNU_PROPERTY_X86_FEATURE_2_* bits should actually be taken into account:
Most of the bits represent advanced (later) features (XSAVE, XSAVEOPT,
and XSAVEC for example being part of v3).
---
The latter part still feels wrong: xstate_xmm is entirely insn operand
dependent, so e.g. an AVX512VL insn would wrongly set baseline when it
really is a v4 insn.

There's more severe bitrotting here when it comes to - first and
foremost - APX, but also other more or less recent additions like
USER-MSR. APX insns like LZCNT with eGPR operand(s) would wrongly set
v3. VEX-encoded URDMSR would also wrongly set v3. EVEX-encoded insns
would wrongly set v4. Question is whether this machinery (a) should be
kept at all, and if so (b) won't better be re-written from scratch.

Comments

Jiang, Haochen Jan. 22, 2025, 7:45 a.m. UTC | #1 
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, January 21, 2025 9:39 PM
> 
> Updating should be based solely on the current instruction. For example,
> recording of VEX-encoded insns as v3 should be independent of there being
> earlier AMX insns.
> 
> Further for BASELINE only a very limited set of the
> GNU_PROPERTY_X86_FEATURE_2_* bits should actually be taken into
> account:
> Most of the bits represent advanced (later) features (XSAVE, XSAVEOPT, and
> XSAVEC for example being part of v3).
> ---
> The latter part still feels wrong: xstate_xmm is entirely insn operand
> dependent, so e.g. an AVX512VL insn would wrongly set baseline when it
> really is a v4 insn.
> 
> There's more severe bitrotting here when it comes to - first and foremost -
> APX, but also other more or less recent additions like USER-MSR. APX insns like
> LZCNT with eGPR operand(s) would wrongly set v3. VEX-encoded URDMSR
> would also wrongly set v3. EVEX-encoded insns would wrongly set v4.
> Question is whether this machinery (a) should be kept at all, and if so (b)
> won't better be re-written from scratch.
> 
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -12173,21 +12173,23 @@ output_insn (const struct last_insn *las
> #ifdef OBJ_ELF
>    if (x86_used_note && now_seg != absolute_section)
>      {
> +      unsigned int feature_2_used = 0;
> +
>        if ((i.xstate & xstate_tmm) == xstate_tmm
>  	  || is_cpu (&i.tm, CpuAMX_TILE))
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_TMM;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_TMM;
> 
>        if (is_cpu (&i.tm, Cpu8087)
>  	  || is_cpu (&i.tm, Cpu287)
>  	  || is_cpu (&i.tm, Cpu387)
>  	  || is_cpu (&i.tm, Cpu687)
>  	  || is_cpu (&i.tm, CpuFISTTP))
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X87;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X87;
> 
>        if ((i.xstate & xstate_mmx)
>  	  || i.tm.mnem_off == MN_emms
>  	  || i.tm.mnem_off == MN_femms)
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MMX;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MMX;
> 
>        if (i.index_reg)
>  	{
> @@ -12210,25 +12212,29 @@ output_insn (const struct last_insn *las
>  		  || is_cpu (&i.tm, CpuAVX)))
>  	  || is_cpu (&i.tm, CpuWideKL)
>  	  || is_cpu (&i.tm, CpuKL))
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XMM;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XMM;
> 
>        if ((i.xstate & xstate_ymm) == xstate_ymm)
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_YMM;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_YMM;
>        if ((i.xstate & xstate_zmm) == xstate_zmm)
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_ZMM;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_ZMM;
>        if (i.mask.reg || (i.xstate & xstate_mask) == xstate_mask)
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MASK;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MASK;
>        if (is_cpu (&i.tm, CpuFXSR))
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_FXSR;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_FXSR;
>        if (is_cpu (&i.tm, CpuXsave))
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVE;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVE;
>        if (is_cpu (&i.tm, CpuXsaveopt))
> -	x86_feature_2_used |=
> GNU_PROPERTY_X86_FEATURE_2_XSAVEOPT;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEOPT;
>        if (is_cpu (&i.tm, CpuXSAVEC))
> -	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEC;
> +	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEC;
> +
> +      x86_feature_2_used |= feature_2_used;
> 
>        if (object_64bit
> -	  || x86_feature_2_used
> +	  || (feature_2_used
> +	      & (GNU_PROPERTY_X86_FEATURE_2_XMM
> +		 | GNU_PROPERTY_X86_FEATURE_2_FXSR)) != 0
>  	  || is_cpu (&i.tm, CpuCMOV)
>  	  || is_cpu (&i.tm, CpuSYSCALL)
>  	  || i.tm.mnem_off == MN_cmpxchg8b)
> @@ -12256,13 +12262,13 @@ output_insn (const struct last_insn *las
>  	      && !is_cpu (&i.tm, CpuFMA4)
>  	      && !is_cpu (&i.tm, CpuLWP)
>  	      && !is_cpu (&i.tm, CpuTBM)
> -	      && !(x86_feature_2_used &
> GNU_PROPERTY_X86_FEATURE_2_TMM))
> +	      && !(feature_2_used & GNU_PROPERTY_X86_FEATURE_2_TMM))
>  	  || is_cpu (&i.tm, CpuF16C)
>  	  || is_cpu (&i.tm, CpuFMA)
>  	  || is_cpu (&i.tm, CpuLZCNT)
>  	  || is_cpu (&i.tm, CpuMovbe)
>  	  || is_cpu (&i.tm, CpuXSAVES)
> -	  || (x86_feature_2_used
> +	  || (feature_2_used
>  	      & (GNU_PROPERTY_X86_FEATURE_2_XSAVE
>  		 | GNU_PROPERTY_X86_FEATURE_2_XSAVEOPT
>  		 | GNU_PROPERTY_X86_FEATURE_2_XSAVEC)) != 0)
> --- a/gas/testsuite/gas/i386/property-6.d
> +++ b/gas/testsuite/gas/i386/property-6.d
> @@ -5,5 +5,5 @@
>  Displaying notes found in: .note.gnu.property
>  [ 	]+Owner[ 	]+Data size[ 	]+Description
>    GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
> -      Properties: x86 ISA used: x86-64-baseline, x86-64-v4
> +      Properties: x86 ISA used: (x86-64-baseline, )?x86-64-v4
>  	x86 feature used: x86, MASK

+ Sunil who is also familiar with might related part in glibc

Sunil, could you also have a look here?

Thx,
Haochen
diff mbox series

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -12173,21 +12173,23 @@  output_insn (const struct last_insn *las
 #ifdef OBJ_ELF
   if (x86_used_note && now_seg != absolute_section)
     {
+      unsigned int feature_2_used = 0;
+
       if ((i.xstate & xstate_tmm) == xstate_tmm
 	  || is_cpu (&i.tm, CpuAMX_TILE))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_TMM;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_TMM;
 
       if (is_cpu (&i.tm, Cpu8087)
 	  || is_cpu (&i.tm, Cpu287)
 	  || is_cpu (&i.tm, Cpu387)
 	  || is_cpu (&i.tm, Cpu687)
 	  || is_cpu (&i.tm, CpuFISTTP))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X87;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_X87;
 
       if ((i.xstate & xstate_mmx)
 	  || i.tm.mnem_off == MN_emms
 	  || i.tm.mnem_off == MN_femms)
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MMX;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MMX;
 
       if (i.index_reg)
 	{
@@ -12210,25 +12212,29 @@  output_insn (const struct last_insn *las
 		  || is_cpu (&i.tm, CpuAVX)))
 	  || is_cpu (&i.tm, CpuWideKL)
 	  || is_cpu (&i.tm, CpuKL))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XMM;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XMM;
 
       if ((i.xstate & xstate_ymm) == xstate_ymm)
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_YMM;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_YMM;
       if ((i.xstate & xstate_zmm) == xstate_zmm)
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_ZMM;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_ZMM;
       if (i.mask.reg || (i.xstate & xstate_mask) == xstate_mask)
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MASK;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_MASK;
       if (is_cpu (&i.tm, CpuFXSR))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_FXSR;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_FXSR;
       if (is_cpu (&i.tm, CpuXsave))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVE;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVE;
       if (is_cpu (&i.tm, CpuXsaveopt))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEOPT;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEOPT;
       if (is_cpu (&i.tm, CpuXSAVEC))
-	x86_feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEC;
+	feature_2_used |= GNU_PROPERTY_X86_FEATURE_2_XSAVEC;
+
+      x86_feature_2_used |= feature_2_used;
 
       if (object_64bit
-	  || x86_feature_2_used
+	  || (feature_2_used
+	      & (GNU_PROPERTY_X86_FEATURE_2_XMM
+		 | GNU_PROPERTY_X86_FEATURE_2_FXSR)) != 0
 	  || is_cpu (&i.tm, CpuCMOV)
 	  || is_cpu (&i.tm, CpuSYSCALL)
 	  || i.tm.mnem_off == MN_cmpxchg8b)
@@ -12256,13 +12262,13 @@  output_insn (const struct last_insn *las
 	      && !is_cpu (&i.tm, CpuFMA4)
 	      && !is_cpu (&i.tm, CpuLWP)
 	      && !is_cpu (&i.tm, CpuTBM)
-	      && !(x86_feature_2_used & GNU_PROPERTY_X86_FEATURE_2_TMM))
+	      && !(feature_2_used & GNU_PROPERTY_X86_FEATURE_2_TMM))
 	  || is_cpu (&i.tm, CpuF16C)
 	  || is_cpu (&i.tm, CpuFMA)
 	  || is_cpu (&i.tm, CpuLZCNT)
 	  || is_cpu (&i.tm, CpuMovbe)
 	  || is_cpu (&i.tm, CpuXSAVES)
-	  || (x86_feature_2_used
+	  || (feature_2_used
 	      & (GNU_PROPERTY_X86_FEATURE_2_XSAVE
 		 | GNU_PROPERTY_X86_FEATURE_2_XSAVEOPT
 		 | GNU_PROPERTY_X86_FEATURE_2_XSAVEC)) != 0)
--- a/gas/testsuite/gas/i386/property-6.d
+++ b/gas/testsuite/gas/i386/property-6.d
@@ -5,5 +5,5 @@ 
 Displaying notes found in: .note.gnu.property
 [ 	]+Owner[ 	]+Data size[ 	]+Description
   GNU                  0x[0-9a-f]+	NT_GNU_PROPERTY_TYPE_0
-      Properties: x86 ISA used: x86-64-baseline, x86-64-v4
+      Properties: x86 ISA used: (x86-64-baseline, )?x86-64-v4
 	x86 feature used: x86, MASK