[PATCH] i386: Fix emissing of __builtin_cpu_supports.

[PATCH] i386: Fix emissing of __builtin_cpu_supports.

[Martin Liška]

The patch fixes __builtin_cpu_supports("avx512vbmi2") which returns a negative
value (that's not allowed in the documentation).

I also checked ppc target that does the same, and __builtin_cpu_is, which
are fine.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin	

	PR target/103661

gcc/ChangeLog:

	* config/i386/i386-builtins.c (fold_builtin_cpu): Compare to 0
	as API expects that non-zero values are returned.  For
	"avx512vbmi2" argument, we return now 1 << 31, which is a
	negative integer value.
---
  gcc/config/i386/i386-builtins.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386-builtins.c b/gcc/config/i386/i386-builtins.c
index 0fb14b55712..7e57b665c1e 100644
--- a/gcc/config/i386/i386-builtins.c
+++ b/gcc/config/i386/i386-builtins.c
@@ -2353,7 +2353,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
        /* Return __cpu_model.__cpu_features[0] & field_val  */
        final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
  		      build_int_cstu (unsigned_type_node, field_val));
-      return build1 (CONVERT_EXPR, integer_type_node, final);
+      return build2 (NE_EXPR, integer_type_node, final,
+		     build_int_cst (unsigned_type_node, 0));
      }
    gcc_unreachable ();
  }
-- 
2.34.1

Re: [PATCH] i386: Fix emissing of __builtin_cpu_supports.

[Jakub Jelinek]

On Tue, Dec 14, 2021 at 10:55:01AM +0100, Martin Liška wrote:
> The patch fixes __builtin_cpu_supports("avx512vbmi2") which returns a negative
> value (that's not allowed in the documentation).
> 
> I also checked ppc target that does the same, and __builtin_cpu_is, which
> are fine.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin	
> 
> 	PR target/103661
> 
> gcc/ChangeLog:
> 
> 	* config/i386/i386-builtins.c (fold_builtin_cpu): Compare to 0
> 	as API expects that non-zero values are returned.  For
> 	"avx512vbmi2" argument, we return now 1 << 31, which is a
> 	negative integer value.
> ---
>  gcc/config/i386/i386-builtins.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/config/i386/i386-builtins.c b/gcc/config/i386/i386-builtins.c
> index 0fb14b55712..7e57b665c1e 100644
> --- a/gcc/config/i386/i386-builtins.c
> +++ b/gcc/config/i386/i386-builtins.c
> @@ -2353,7 +2353,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
>        /* Return __cpu_model.__cpu_features[0] & field_val  */
>        final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
>  		      build_int_cstu (unsigned_type_node, field_val));
> -      return build1 (CONVERT_EXPR, integer_type_node, final);
> +      return build2 (NE_EXPR, integer_type_node, final,
> +		     build_int_cst (unsigned_type_node, 0));
>      }
>    gcc_unreachable ();
>  }

Wouldn't this be better done only if field_val has the msb set
and keep the CONVERT_EXPR otherwise (why isn't it NOP_EXPR?)?

	Jakub

Re: [PATCH] i386: Fix emissing of __builtin_cpu_supports.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On 12/14/21 11:28, Jakub Jelinek wrote:
> Wouldn't this be better done only if field_val has the msb set

Yes, updated in the attached patch.

> and keep the CONVERT_EXPR otherwise (why isn't it NOP_EXPR?)?

Dunno, but I can prepare a separate patch (likely stage1 material,
right)? Note that are other places that also use CONVERT_EXPR.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-i386-Fix-emissing-of-__builtin_cpu_supports.patch --]
[-- Type: text/x-patch, Size: 1342 bytes --]

From 227450e9f3a506fdfcff67aa45135fe31f3f91f6 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 13 Dec 2021 15:34:30 +0100
Subject: [PATCH] i386: Fix emissing of __builtin_cpu_supports.

	PR target/103661

gcc/ChangeLog:

	* config/i386/i386-builtins.c (fold_builtin_cpu): Compare to 0
	as API expects that non-zero values are returned (do that
	it mask == 31).
	For "avx512vbmi2" argument, we return now 1 << 31, which is a
	negative integer value.
---
 gcc/config/i386/i386-builtins.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386-builtins.c b/gcc/config/i386/i386-builtins.c
index 0fb14b55712..bca244fc011 100644
--- a/gcc/config/i386/i386-builtins.c
+++ b/gcc/config/i386/i386-builtins.c
@@ -2353,7 +2353,11 @@ fold_builtin_cpu (tree fndecl, tree *args)
       /* Return __cpu_model.__cpu_features[0] & field_val  */
       final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
 		      build_int_cstu (unsigned_type_node, field_val));
-      return build1 (CONVERT_EXPR, integer_type_node, final);
+      if (isa_names_table[i].feature == 31)
+	return build2 (NE_EXPR, integer_type_node, final,
+		       build_int_cst (unsigned_type_node, 0));
+      else
+	return build1 (CONVERT_EXPR, integer_type_node, final);
     }
   gcc_unreachable ();
 }
-- 
2.34.1

Re: [PATCH] i386: Fix emissing of __builtin_cpu_supports.

[Jakub Jelinek]

On Tue, Dec 14, 2021 at 04:07:55PM +0100, Martin Liška wrote:
> On 12/14/21 11:28, Jakub Jelinek wrote:
> > Wouldn't this be better done only if field_val has the msb set
> 
> Yes, updated in the attached patch.
> 
> > and keep the CONVERT_EXPR otherwise (why isn't it NOP_EXPR?)?
> 
> Dunno, but I can prepare a separate patch (likely stage1 material,
> right)? Note that are other places that also use CONVERT_EXPR.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin

> From 227450e9f3a506fdfcff67aa45135fe31f3f91f6 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Mon, 13 Dec 2021 15:34:30 +0100
> Subject: [PATCH] i386: Fix emissing of __builtin_cpu_supports.
> 
> 	PR target/103661
> 
> gcc/ChangeLog:
> 
> 	* config/i386/i386-builtins.c (fold_builtin_cpu): Compare to 0
> 	as API expects that non-zero values are returned (do that
> 	it mask == 31).
> 	For "avx512vbmi2" argument, we return now 1 << 31, which is a
> 	negative integer value.
> ---
>  gcc/config/i386/i386-builtins.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/config/i386/i386-builtins.c b/gcc/config/i386/i386-builtins.c
> index 0fb14b55712..bca244fc011 100644
> --- a/gcc/config/i386/i386-builtins.c
> +++ b/gcc/config/i386/i386-builtins.c
> @@ -2353,7 +2353,11 @@ fold_builtin_cpu (tree fndecl, tree *args)
>        /* Return __cpu_model.__cpu_features[0] & field_val  */
>        final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
>  		      build_int_cstu (unsigned_type_node, field_val));
> -      return build1 (CONVERT_EXPR, integer_type_node, final);
> +      if (isa_names_table[i].feature == 31)
> +	return build2 (NE_EXPR, integer_type_node, final,
> +		       build_int_cst (unsigned_type_node, 0));
> +      else
> +	return build1 (CONVERT_EXPR, integer_type_node, final);
>      }
>    gcc_unreachable ();
>  }

I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.

	Jakub

[PATCH] i386: simplify cpu_feature handling

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On 12/14/21 17:12, Jakub Jelinek wrote:
> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.

Installed with that change, thanks.

Moreover, I'm suggesting a simplification:

The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
that can be simplified with NOP_EXPR.

Survives i386.exp tests, may I install the patch after testing or
is it a stage1 material?

Thanks,
Martin

[-- Attachment #2: 0001-i386-simplify-cpu_feature-handling.patch --]
[-- Type: text/x-patch, Size: 7216 bytes --]

From 9fa89df81b3e6cb56f6ab59b0993168e7a048489 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 15 Dec 2021 10:54:23 +0100
Subject: [PATCH] i386: simplify cpu_feature handling

The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
that can be simplified with NOP_EXPR.

gcc/ChangeLog:

	* common/config/i386/cpuinfo.h (has_cpu_feature): Directly
	compute index in cpu_features2.
	(set_cpu_feature): Likewise.
	* config/i386/i386-builtins.c (fold_builtin_cpu): Also remove
	loop for cpu_features2 and use NOP_EXPRs.
---
 gcc/common/config/i386/cpuinfo.h | 50 +++++++++++---------
 gcc/config/i386/i386-builtins.c  | 79 ++++++++++++++++----------------
 2 files changed, 67 insertions(+), 62 deletions(-)

diff --git a/gcc/common/config/i386/cpuinfo.h b/gcc/common/config/i386/cpuinfo.h
index bbf29bdb116..dd321920108 100644
--- a/gcc/common/config/i386/cpuinfo.h
+++ b/gcc/common/config/i386/cpuinfo.h
@@ -55,43 +55,49 @@ struct __processor_model2
 static inline int
 has_cpu_feature (struct __processor_model *cpu_model,
 		 unsigned int *cpu_features2,
-		 enum processor_features f)
+		 enum processor_features feature)
 {
-  unsigned int i;
+  unsigned index, offset;
+  unsigned f = feature;
+
   if (f < 32)
     {
       /* The first 32 features.  */
-      return cpu_model->__cpu_features[0] & (1U << (f & 31));
+      return cpu_model->__cpu_features[0] & (1U << f);
+    }
+  else
+    {
+      /* The rest of features.  cpu_features2[i] contains features from
+	 (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
+      f -= 32;
+      index = f / 32;
+      offset = f % 32;
+      return cpu_features2[index] & (1U << offset);
     }
-  /* The rest of features.  cpu_features2[i] contains features from
-     (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
-  for (i = 0; i < SIZE_OF_CPU_FEATURES; i++)
-    if (f < (32 + 32 + i * 32))
-    return cpu_features2[i] & (1U << ((f - (32 + i * 32)) & 31));
-  gcc_unreachable ();
 }
 
 static inline void
 set_cpu_feature (struct __processor_model *cpu_model,
 		 unsigned int *cpu_features2,
-		 enum processor_features f)
+		 enum processor_features feature)
 {
-  unsigned int i;
+  unsigned index, offset;
+  unsigned f = feature;
+
   if (f < 32)
     {
       /* The first 32 features.  */
-      cpu_model->__cpu_features[0] |= (1U << (f & 31));
-      return;
+      cpu_model->__cpu_features[0] |= (1U << f);
+    }
+  else
+    {
+      /* The rest of features.  cpu_features2[i] contains features from
+	 (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
+      f -= 32;
+      index = f / 32;
+      offset = f % 32;
+      cpu_features2[index] |= (1U << offset);
     }
-  /* The rest of features.  cpu_features2[i] contains features from
-     (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
-  for (i = 0; i < SIZE_OF_CPU_FEATURES; i++)
-    if (f < (32 + 32 + i * 32))
-      {
-	cpu_features2[i] |= (1U << ((f - (32 + i * 32)) & 31));
-	return;
-      }
-  gcc_unreachable ();
 }
 
 /* Get the specific type of AMD CPU and return AMD CPU name.  Return
diff --git a/gcc/config/i386/i386-builtins.c b/gcc/config/i386/i386-builtins.c
index 4b30cf75c26..31e034e1bc9 100644
--- a/gcc/config/i386/i386-builtins.c
+++ b/gcc/config/i386/i386-builtins.c
@@ -2275,7 +2275,7 @@ fold_builtin_cpu (tree fndecl, tree *args)
       /* Check the value.  */
       final = build2 (EQ_EXPR, unsigned_type_node, ref,
 		      build_int_cstu (unsigned_type_node, field_val));
-      return build1 (CONVERT_EXPR, integer_type_node, final);
+      return build1 (NOP_EXPR, integer_type_node, final);
     }
   else if (fn_code == IX86_BUILTIN_CPU_SUPPORTS)
     {
@@ -2300,7 +2300,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
 	  return integer_zero_node;
 	}
 
-      if (isa_names_table[i].feature >= 32)
+      unsigned feature = isa_names_table[i].feature;
+      if (feature >= INT_TYPE_SIZE)
 	{
 	  if (ix86_cpu_features2_var == nullptr)
 	    {
@@ -2318,46 +2319,44 @@ fold_builtin_cpu (tree fndecl, tree *args)
 	      varpool_node::add (ix86_cpu_features2_var);
 	    }
 
-	  for (unsigned int j = 0; j < SIZE_OF_CPU_FEATURES; j++)
-	    if (isa_names_table[i].feature < (32 + 32 + j * 32))
-	      {
-		field_val = (1U << (isa_names_table[i].feature
-				    - (32 + j * 32)));
-		tree index = size_int (j);
-		array_elt = build4 (ARRAY_REF, unsigned_type_node,
-				    ix86_cpu_features2_var,
-				    index, NULL_TREE, NULL_TREE);
-		/* Return __cpu_features2[index] & field_val  */
-		final = build2 (BIT_AND_EXPR, unsigned_type_node,
-				array_elt,
-				build_int_cstu (unsigned_type_node,
-						field_val));
-		return build1 (CONVERT_EXPR, integer_type_node, final);
-	      }
+	  feature -= INT_TYPE_SIZE;
+	  field_val = 1U << (feature % INT_TYPE_SIZE);
+	  tree index = size_int (feature / INT_TYPE_SIZE);
+	  array_elt = build4 (ARRAY_REF, unsigned_type_node,
+			      ix86_cpu_features2_var,
+			      index, NULL_TREE, NULL_TREE);
+	  /* Return __cpu_features2[index] & field_val  */
+	  final = build2 (BIT_AND_EXPR, unsigned_type_node,
+			  array_elt,
+			  build_int_cstu (unsigned_type_node,
+					  field_val));
+	  return build1 (NOP_EXPR, integer_type_node, final);
 	}
-
-      field = TYPE_FIELDS (ix86_cpu_model_type_node);
-      /* Get the last field, which is __cpu_features.  */
-      while (DECL_CHAIN (field))
-        field = DECL_CHAIN (field);
-
-      /* Get the appropriate field: __cpu_model.__cpu_features  */
-      ref = build3 (COMPONENT_REF, TREE_TYPE (field), ix86_cpu_model_var,
-		    field, NULL_TREE);
-
-      /* Access the 0th element of __cpu_features array.  */
-      array_elt = build4 (ARRAY_REF, unsigned_type_node, ref,
-			  integer_zero_node, NULL_TREE, NULL_TREE);
-
-      field_val = (1U << isa_names_table[i].feature);
-      /* Return __cpu_model.__cpu_features[0] & field_val  */
-      final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
-		      build_int_cstu (unsigned_type_node, field_val));
-      if (isa_names_table[i].feature == (INT_TYPE_SIZE - 1))
-	return build2 (NE_EXPR, integer_type_node, final,
-		       build_int_cst (unsigned_type_node, 0));
       else
-	return build1 (CONVERT_EXPR, integer_type_node, final);
+	{
+	  field = TYPE_FIELDS (ix86_cpu_model_type_node);
+	  /* Get the last field, which is __cpu_features.  */
+	  while (DECL_CHAIN (field))
+	    field = DECL_CHAIN (field);
+
+	  /* Get the appropriate field: __cpu_model.__cpu_features  */
+	  ref = build3 (COMPONENT_REF, TREE_TYPE (field), ix86_cpu_model_var,
+			field, NULL_TREE);
+
+	  /* Access the 0th element of __cpu_features array.  */
+	  array_elt = build4 (ARRAY_REF, unsigned_type_node, ref,
+			      integer_zero_node, NULL_TREE, NULL_TREE);
+
+	  field_val = (1U << feature);
+	  /* Return __cpu_model.__cpu_features[0] & field_val  */
+	  final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
+			  build_int_cstu (unsigned_type_node, field_val));
+	  if (feature == (INT_TYPE_SIZE - 1))
+	    return build2 (NE_EXPR, integer_type_node, final,
+			   build_int_cst (unsigned_type_node, 0));
+	  else
+	    return build1 (NOP_EXPR, integer_type_node, final);
+	}
     }
   gcc_unreachable ();
 }
-- 
2.34.1

Re: [PATCH] i386: simplify cpu_feature handling

[Stefan Kneifel]

Am 15.12.21 um 10:57 schrieb Martin Liška:
> On 12/14/21 17:12, Jakub Jelinek wrote:
>> I'd use INT_TYPE_SIZE - 1 instead of 31. Otherwise LGTM.
>
> Installed with that change, thanks.
>
> Moreover, I'm suggesting a simplification:
>
> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
> that can be simplified with NOP_EXPR.
>
> Survives i386.exp tests, may I install the patch after testing or
> is it a stage1 material?
>
> Thanks,
> Martin

The loops indeed seem to be unnecessary.

For safety reasons: what would you think about throwing an ICE if (index 
 >= SIZE_OF_CPU_FEATURES) ?
This should not happen - however, a lot of things shouldn't happen... 
and it might facilitiate locating a potential bug at a later time.

Regards, Stefan

Re: [PATCH] i386: simplify cpu_feature handling

[Martin Liška]

On 12/16/21 21:58, Stefan Kneifel wrote:
> Am 15.12.21 um 10:57 schrieb Martin Liška:
>> On 12/14/21 17:12, Jakub Jelinek wrote:
>>> I'd use INT_TYPE_SIZE - 1 instead of 31. Otherwise LGTM.
>>
>> Installed with that change, thanks.
>>
>> Moreover, I'm suggesting a simplification:
>>
>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
>> that can be simplified with NOP_EXPR.
>>
>> Survives i386.exp tests, may I install the patch after testing or
>> is it a stage1 material?
>>
>> Thanks,
>> Martin
> 
> The loops indeed seem to be unnecessary.
> 
> For safety reasons: what would you think about throwing an ICE if (index >= SIZE_OF_CPU_FEATURES) ?
> This should not happen - however, a lot of things shouldn't happen... and it might facilitiate locating a potential bug at a later time.

Hello.

Well, I see your point, but I don't think it's necessary as the macro is well defined.
Note we have a ASAN and UBSAN bootstrap that would caught such an error.

Cheers,
Martin

> 
> Regards, Stefan
> 

Re: [PATCH] i386: simplify cpu_feature handling

[Martin Liška]

PING: Jakub?

On 12/15/21 10:57, Martin Liška wrote:
> On 12/14/21 17:12, Jakub Jelinek wrote:
>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
> 
> Installed with that change, thanks.
> 
> Moreover, I'm suggesting a simplification:
> 
> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
> that can be simplified with NOP_EXPR.
> 
> Survives i386.exp tests, may I install the patch after testing or
> is it a stage1 material?
> 
> Thanks,
> Martin

Re: [PATCH] i386: simplify cpu_feature handling

[Martin Liška]

@Jakub: May I install it once stage1 opens?

Cheers,
Martin

On 1/3/22 12:43, Martin Liška wrote:
> PING: Jakub?
> 
> On 12/15/21 10:57, Martin Liška wrote:
>> On 12/14/21 17:12, Jakub Jelinek wrote:
>>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
>>
>> Installed with that change, thanks.
>>
>> Moreover, I'm suggesting a simplification:
>>
>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
>> that can be simplified with NOP_EXPR.
>>
>> Survives i386.exp tests, may I install the patch after testing or
>> is it a stage1 material?
>>
>> Thanks,
>> Martin
> 

Re: [PATCH] i386: simplify cpu_feature handling

[Martin Liška]

On 3/31/22 09:01, Martin Liška wrote:
> @Jakub: May I install it once stage1 opens?

May I please ping this?

Thanks,
Martin

> 
> Cheers,
> Martin
> 
> On 1/3/22 12:43, Martin Liška wrote:
>> PING: Jakub?
>>
>> On 12/15/21 10:57, Martin Liška wrote:
>>> On 12/14/21 17:12, Jakub Jelinek wrote:
>>>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
>>>
>>> Installed with that change, thanks.
>>>
>>> Moreover, I'm suggesting a simplification:
>>>
>>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
>>> that can be simplified with NOP_EXPR.
>>>
>>> Survives i386.exp tests, may I install the patch after testing or
>>> is it a stage1 material?
>>>
>>> Thanks,
>>> Martin
>>
> 

Re: [PATCH] i386: simplify cpu_feature handling

[Martin Liška]

On 5/2/22 09:57, Martin Liška wrote:
> On 3/31/22 09:01, Martin Liška wrote:
>> @Jakub: May I install it once stage1 opens?
> 
> May I please ping this?
> 
> Thanks,
> Martin
> 
>>
>> Cheers,
>> Martin
>>
>> On 1/3/22 12:43, Martin Liška wrote:
>>> PING: Jakub?
>>>
>>> On 12/15/21 10:57, Martin Liška wrote:
>>>> On 12/14/21 17:12, Jakub Jelinek wrote:
>>>>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
>>>>
>>>> Installed with that change, thanks.
>>>>
>>>> Moreover, I'm suggesting a simplification:
>>>>
>>>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
>>>> that can be simplified with NOP_EXPR.
>>>>
>>>> Survives i386.exp tests, may I install the patch after testing or
>>>> is it a stage1 material?
>>>>
>>>> Thanks,
>>>> Martin
>>>
>>
> 

CCing Uros.

May I install the patch?

Martin

Re: [PATCH] i386: simplify cpu_feature handling

[Uros Bizjak]

On Wed, May 11, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/2/22 09:57, Martin Liška wrote:
> > On 3/31/22 09:01, Martin Liška wrote:
> >> @Jakub: May I install it once stage1 opens?
> >
> > May I please ping this?
> >
> > Thanks,
> > Martin
> >
> >>
> >> Cheers,
> >> Martin
> >>
> >> On 1/3/22 12:43, Martin Liška wrote:
> >>> PING: Jakub?
> >>>
> >>> On 12/15/21 10:57, Martin Liška wrote:
> >>>> On 12/14/21 17:12, Jakub Jelinek wrote:
> >>>>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
> >>>>
> >>>> Installed with that change, thanks.
> >>>>
> >>>> Moreover, I'm suggesting a simplification:
> >>>>
> >>>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
> >>>> that can be simplified with NOP_EXPR.
> >>>>
> >>>> Survives i386.exp tests, may I install the patch after testing or
> >>>> is it a stage1 material?
> >>>>
> >>>> Thanks,
> >>>> Martin
> >>>
> >>
> >
>
> CCing Uros.
>
> May I install the patch?

Can you please attach the latest version of the patch? I only found
the version from Dec. 15. 2021, which I'm not sure is the latest,

Thanks,
Uros.

Re: [PATCH] i386: simplify cpu_feature handling

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On 5/11/22 10:27, Uros Bizjak wrote:
> On Wed, May 11, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 5/2/22 09:57, Martin Liška wrote:
>>> On 3/31/22 09:01, Martin Liška wrote:
>>>> @Jakub: May I install it once stage1 opens?
>>>
>>> May I please ping this?
>>>
>>> Thanks,
>>> Martin
>>>
>>>>
>>>> Cheers,
>>>> Martin
>>>>
>>>> On 1/3/22 12:43, Martin Liška wrote:
>>>>> PING: Jakub?
>>>>>
>>>>> On 12/15/21 10:57, Martin Liška wrote:
>>>>>> On 12/14/21 17:12, Jakub Jelinek wrote:
>>>>>>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
>>>>>>
>>>>>> Installed with that change, thanks.
>>>>>>
>>>>>> Moreover, I'm suggesting a simplification:
>>>>>>
>>>>>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
>>>>>> that can be simplified with NOP_EXPR.
>>>>>>
>>>>>> Survives i386.exp tests, may I install the patch after testing or
>>>>>> is it a stage1 material?
>>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>>
>>>>
>>>
>>
>> CCing Uros.
>>
>> May I install the patch?
> 
> Can you please attach the latest version of the patch? I only found
> the version from Dec. 15. 2021, which I'm not sure is the latest,
> 
> Thanks,
> Uros.

Sure, there's the rebased version of the patch.

Thanks,
Martin

[-- Attachment #2: 0001-i386-simplify-cpu_feature-handling.patch --]
[-- Type: text/x-patch, Size: 7221 bytes --]

From 72aa0459f84baa845fef7f9f7f0d04edda410d87 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 15 Dec 2021 10:54:23 +0100
Subject: [PATCH] i386: simplify cpu_feature handling

The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
that can be simplified with NOP_EXPR.

gcc/ChangeLog:

	* common/config/i386/cpuinfo.h (has_cpu_feature): Directly
	compute index in cpu_features2.
	(set_cpu_feature): Likewise.
	* config/i386/i386-builtins.cc (fold_builtin_cpu): Also remove
	loop for cpu_features2 and use NOP_EXPRs.
---
 gcc/common/config/i386/cpuinfo.h | 50 +++++++++++---------
 gcc/config/i386/i386-builtins.cc | 79 ++++++++++++++++----------------
 2 files changed, 67 insertions(+), 62 deletions(-)

diff --git a/gcc/common/config/i386/cpuinfo.h b/gcc/common/config/i386/cpuinfo.h
index 239759dc766..6d6171f4555 100644
--- a/gcc/common/config/i386/cpuinfo.h
+++ b/gcc/common/config/i386/cpuinfo.h
@@ -55,43 +55,49 @@ struct __processor_model2
 static inline int
 has_cpu_feature (struct __processor_model *cpu_model,
 		 unsigned int *cpu_features2,
-		 enum processor_features f)
+		 enum processor_features feature)
 {
-  unsigned int i;
+  unsigned index, offset;
+  unsigned f = feature;
+
   if (f < 32)
     {
       /* The first 32 features.  */
-      return cpu_model->__cpu_features[0] & (1U << (f & 31));
+      return cpu_model->__cpu_features[0] & (1U << f);
+    }
+  else
+    {
+      /* The rest of features.  cpu_features2[i] contains features from
+	 (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
+      f -= 32;
+      index = f / 32;
+      offset = f % 32;
+      return cpu_features2[index] & (1U << offset);
     }
-  /* The rest of features.  cpu_features2[i] contains features from
-     (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
-  for (i = 0; i < SIZE_OF_CPU_FEATURES; i++)
-    if (f < (32 + 32 + i * 32))
-    return cpu_features2[i] & (1U << ((f - (32 + i * 32)) & 31));
-  gcc_unreachable ();
 }
 
 static inline void
 set_cpu_feature (struct __processor_model *cpu_model,
 		 unsigned int *cpu_features2,
-		 enum processor_features f)
+		 enum processor_features feature)
 {
-  unsigned int i;
+  unsigned index, offset;
+  unsigned f = feature;
+
   if (f < 32)
     {
       /* The first 32 features.  */
-      cpu_model->__cpu_features[0] |= (1U << (f & 31));
-      return;
+      cpu_model->__cpu_features[0] |= (1U << f);
+    }
+  else
+    {
+      /* The rest of features.  cpu_features2[i] contains features from
+	 (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
+      f -= 32;
+      index = f / 32;
+      offset = f % 32;
+      cpu_features2[index] |= (1U << offset);
     }
-  /* The rest of features.  cpu_features2[i] contains features from
-     (32 + i * 32) to (31 + 32 + i * 32), inclusively.  */
-  for (i = 0; i < SIZE_OF_CPU_FEATURES; i++)
-    if (f < (32 + 32 + i * 32))
-      {
-	cpu_features2[i] |= (1U << ((f - (32 + i * 32)) & 31));
-	return;
-      }
-  gcc_unreachable ();
 }
 
 /* Get the specific type of AMD CPU and return AMD CPU name.  Return
diff --git a/gcc/config/i386/i386-builtins.cc b/gcc/config/i386/i386-builtins.cc
index 8c6d0fe9631..59c7da25a14 100644
--- a/gcc/config/i386/i386-builtins.cc
+++ b/gcc/config/i386/i386-builtins.cc
@@ -2280,7 +2280,7 @@ fold_builtin_cpu (tree fndecl, tree *args)
       /* Check the value.  */
       final = build2 (EQ_EXPR, unsigned_type_node, ref,
 		      build_int_cstu (unsigned_type_node, field_val));
-      return build1 (CONVERT_EXPR, integer_type_node, final);
+      return build1 (NOP_EXPR, integer_type_node, final);
     }
   else if (fn_code == IX86_BUILTIN_CPU_SUPPORTS)
     {
@@ -2305,7 +2305,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
 	  return integer_zero_node;
 	}
 
-      if (isa_names_table[i].feature >= 32)
+      unsigned feature = isa_names_table[i].feature;
+      if (feature >= INT_TYPE_SIZE)
 	{
 	  if (ix86_cpu_features2_var == nullptr)
 	    {
@@ -2323,46 +2324,44 @@ fold_builtin_cpu (tree fndecl, tree *args)
 	      varpool_node::add (ix86_cpu_features2_var);
 	    }
 
-	  for (unsigned int j = 0; j < SIZE_OF_CPU_FEATURES; j++)
-	    if (isa_names_table[i].feature < (32 + 32 + j * 32))
-	      {
-		field_val = (1U << (isa_names_table[i].feature
-				    - (32 + j * 32)));
-		tree index = size_int (j);
-		array_elt = build4 (ARRAY_REF, unsigned_type_node,
-				    ix86_cpu_features2_var,
-				    index, NULL_TREE, NULL_TREE);
-		/* Return __cpu_features2[index] & field_val  */
-		final = build2 (BIT_AND_EXPR, unsigned_type_node,
-				array_elt,
-				build_int_cstu (unsigned_type_node,
-						field_val));
-		return build1 (CONVERT_EXPR, integer_type_node, final);
-	      }
+	  feature -= INT_TYPE_SIZE;
+	  field_val = 1U << (feature % INT_TYPE_SIZE);
+	  tree index = size_int (feature / INT_TYPE_SIZE);
+	  array_elt = build4 (ARRAY_REF, unsigned_type_node,
+			      ix86_cpu_features2_var,
+			      index, NULL_TREE, NULL_TREE);
+	  /* Return __cpu_features2[index] & field_val  */
+	  final = build2 (BIT_AND_EXPR, unsigned_type_node,
+			  array_elt,
+			  build_int_cstu (unsigned_type_node,
+					  field_val));
+	  return build1 (NOP_EXPR, integer_type_node, final);
 	}
-
-      field = TYPE_FIELDS (ix86_cpu_model_type_node);
-      /* Get the last field, which is __cpu_features.  */
-      while (DECL_CHAIN (field))
-        field = DECL_CHAIN (field);
-
-      /* Get the appropriate field: __cpu_model.__cpu_features  */
-      ref = build3 (COMPONENT_REF, TREE_TYPE (field), ix86_cpu_model_var,
-		    field, NULL_TREE);
-
-      /* Access the 0th element of __cpu_features array.  */
-      array_elt = build4 (ARRAY_REF, unsigned_type_node, ref,
-			  integer_zero_node, NULL_TREE, NULL_TREE);
-
-      field_val = (1U << isa_names_table[i].feature);
-      /* Return __cpu_model.__cpu_features[0] & field_val  */
-      final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
-		      build_int_cstu (unsigned_type_node, field_val));
-      if (isa_names_table[i].feature == (INT_TYPE_SIZE - 1))
-	return build2 (NE_EXPR, integer_type_node, final,
-		       build_int_cst (unsigned_type_node, 0));
       else
-	return build1 (CONVERT_EXPR, integer_type_node, final);
+	{
+	  field = TYPE_FIELDS (ix86_cpu_model_type_node);
+	  /* Get the last field, which is __cpu_features.  */
+	  while (DECL_CHAIN (field))
+	    field = DECL_CHAIN (field);
+
+	  /* Get the appropriate field: __cpu_model.__cpu_features  */
+	  ref = build3 (COMPONENT_REF, TREE_TYPE (field), ix86_cpu_model_var,
+			field, NULL_TREE);
+
+	  /* Access the 0th element of __cpu_features array.  */
+	  array_elt = build4 (ARRAY_REF, unsigned_type_node, ref,
+			      integer_zero_node, NULL_TREE, NULL_TREE);
+
+	  field_val = (1U << feature);
+	  /* Return __cpu_model.__cpu_features[0] & field_val  */
+	  final = build2 (BIT_AND_EXPR, unsigned_type_node, array_elt,
+			  build_int_cstu (unsigned_type_node, field_val));
+	  if (feature == (INT_TYPE_SIZE - 1))
+	    return build2 (NE_EXPR, integer_type_node, final,
+			   build_int_cst (unsigned_type_node, 0));
+	  else
+	    return build1 (NOP_EXPR, integer_type_node, final);
+	}
     }
   gcc_unreachable ();
 }
-- 
2.36.1

Re: [PATCH] i386: simplify cpu_feature handling

[Uros Bizjak]

On Wed, May 11, 2022 at 10:50 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 5/11/22 10:27, Uros Bizjak wrote:
> > On Wed, May 11, 2022 at 10:19 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 5/2/22 09:57, Martin Liška wrote:
> >>> On 3/31/22 09:01, Martin Liška wrote:
> >>>> @Jakub: May I install it once stage1 opens?
> >>>
> >>> May I please ping this?
> >>>
> >>> Thanks,
> >>> Martin
> >>>
> >>>>
> >>>> Cheers,
> >>>> Martin
> >>>>
> >>>> On 1/3/22 12:43, Martin Liška wrote:
> >>>>> PING: Jakub?
> >>>>>
> >>>>> On 12/15/21 10:57, Martin Liška wrote:
> >>>>>> On 12/14/21 17:12, Jakub Jelinek wrote:
> >>>>>>> I'd use INT_TYPE_SIZE - 1 instead of 31.  Otherwise LGTM.
> >>>>>>
> >>>>>> Installed with that change, thanks.
> >>>>>>
> >>>>>> Moreover, I'm suggesting a simplification:
> >>>>>>
> >>>>>> The patch removes unneeded loops for cpu_features2 and CONVERT_EXPR
> >>>>>> that can be simplified with NOP_EXPR.
> >>>>>>
> >>>>>> Survives i386.exp tests, may I install the patch after testing or
> >>>>>> is it a stage1 material?
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Martin
> >>>>>
> >>>>
> >>>
> >>
> >> CCing Uros.
> >>
> >> May I install the patch?
> >
> > Can you please attach the latest version of the patch? I only found
> > the version from Dec. 15. 2021, which I'm not sure is the latest,
> >
> > Thanks,
> > Uros.
>
> Sure, there's the rebased version of the patch.

LGTM.

Thanks,
Uros.