middle-end, v2: Add new value for vector types for __builtin_classify_type

Message ID ZVdy3Dpq5YryeUpb@tucnak
State Unresolved
Headers
Series middle-end, v2: Add new value for vector types for __builtin_classify_type |

Checks

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

Commit Message

Jakub Jelinek Nov. 17, 2023, 2:04 p.m. UTC
  On Thu, Nov 16, 2023 at 11:49:03PM +0000, Joseph Myers wrote:
> > > While filing a clang request to return 18 on _BitInts for
> > > __builtin_classify_type instead of -1 they return currently, I've
> > > noticed that we return -1 for vector types.  I'm not convinced it is a good
> > > idea to change behavior of __builtin_classify_type (vector_expression)
> > > after 22 years it behaved one way (returned -1), but the
> > > __builtin_classify_type (type) form is a new extension added for GCC 14,
> > > so this patch returns 19 for vectors just in that second form.  Many other
> > > return values are only accessible from the second form as well (mostly
> > > because
> > > of argument promotions), so I think it is fine like that.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > The C++ changes are OK (and obvious).  I'm skeptical of the choice to keep
> > returning -1 for the expression form, it seems more likely to cause problems
> > (due to it disagreeing with the type form) than changing it (due to old code
> > somehow relying on -1?).  But people who are more familiar with the use of
> > __builtin_classify_type should make the call.
> 
> I'm also doubtful of keeping returning -1 for vectors in expression form 
> (I'd be surprised if people are actually using __builtin_classify_type 
> with vectors).  The C changes are OK (but the front-end changes wouldn't 
> be needed at all if the vector and type argument cases aren't 
> distinguished).

Given that you've both agreed that you think changing
__builtin_classify_type (vector_expr) is ok (hopefully in the wild people
test bct against known values rather than pretending to know what the
unknown -1 means because it can mean multiple type kinds), here is a simpler
patch which changes it even for that.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-11-17  Jakub Jelinek  <jakub@redhat.com>

gcc/
	* typeclass.h (enum type_class): Add vector_type_class.
	* builtins.cc (type_to_class): Return vector_type_class for
	VECTOR_TYPE.
gcc/testsuite/
	* c-c++-common/builtin-classify-type-1.c (main): Add tests for vector
	types.



	Jakub
  

Comments

Richard Biener Nov. 20, 2023, 8:03 a.m. UTC | #1
On Fri, 17 Nov 2023, Jakub Jelinek wrote:

> On Thu, Nov 16, 2023 at 11:49:03PM +0000, Joseph Myers wrote:
> > > > While filing a clang request to return 18 on _BitInts for
> > > > __builtin_classify_type instead of -1 they return currently, I've
> > > > noticed that we return -1 for vector types.  I'm not convinced it is a good
> > > > idea to change behavior of __builtin_classify_type (vector_expression)
> > > > after 22 years it behaved one way (returned -1), but the
> > > > __builtin_classify_type (type) form is a new extension added for GCC 14,
> > > > so this patch returns 19 for vectors just in that second form.  Many other
> > > > return values are only accessible from the second form as well (mostly
> > > > because
> > > > of argument promotions), so I think it is fine like that.
> > > > 
> > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > > 
> > > The C++ changes are OK (and obvious).  I'm skeptical of the choice to keep
> > > returning -1 for the expression form, it seems more likely to cause problems
> > > (due to it disagreeing with the type form) than changing it (due to old code
> > > somehow relying on -1?).  But people who are more familiar with the use of
> > > __builtin_classify_type should make the call.
> > 
> > I'm also doubtful of keeping returning -1 for vectors in expression form 
> > (I'd be surprised if people are actually using __builtin_classify_type 
> > with vectors).  The C changes are OK (but the front-end changes wouldn't 
> > be needed at all if the vector and type argument cases aren't 
> > distinguished).
> 
> Given that you've both agreed that you think changing
> __builtin_classify_type (vector_expr) is ok (hopefully in the wild people
> test bct against known values rather than pretending to know what the
> unknown -1 means because it can mean multiple type kinds), here is a simpler
> patch which changes it even for that.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.  Do we have to adjust any of our documentation for this?

> 2023-11-17  Jakub Jelinek  <jakub@redhat.com>
> 
> gcc/
> 	* typeclass.h (enum type_class): Add vector_type_class.
> 	* builtins.cc (type_to_class): Return vector_type_class for
> 	VECTOR_TYPE.
> gcc/testsuite/
> 	* c-c++-common/builtin-classify-type-1.c (main): Add tests for vector
> 	types.
> 
> --- gcc/typeclass.h.jj	2023-09-06 17:28:24.238977355 +0200
> +++ gcc/typeclass.h	2023-11-10 10:50:59.519007647 +0100
> @@ -38,7 +38,7 @@ enum type_class
>    record_type_class, union_type_class,
>    array_type_class, string_type_class,
>    lang_type_class, opaque_type_class,
> -  bitint_type_class
> +  bitint_type_class, vector_type_class
>  };
>  
>  #endif /* GCC_TYPECLASS_H */
> --- gcc/builtins.cc.jj	2023-11-09 09:17:40.230182483 +0100
> +++ gcc/builtins.cc	2023-11-10 11:19:29.669129855 +0100
> @@ -1859,6 +1859,7 @@ type_to_class (tree type)
>      case LANG_TYPE:	   return lang_type_class;
>      case OPAQUE_TYPE:      return opaque_type_class;
>      case BITINT_TYPE:	   return bitint_type_class;
> +    case VECTOR_TYPE:	   return vector_type_class;
>      default:		   return no_type_class;
>      }
>  }
> --- gcc/testsuite/c-c++-common/builtin-classify-type-1.c.jj	2023-09-26 09:25:30.019599039 +0200
> +++ gcc/testsuite/c-c++-common/builtin-classify-type-1.c	2023-11-10 11:02:01.927776922 +0100
> @@ -22,6 +22,10 @@ main ()
>    const char *p = (const char *) 0;
>    float f = 0.0;
>    _Complex double c = 0.0;
> +  typedef int VI __attribute__((vector_size (4 * sizeof (int))));
> +  typedef float VF __attribute__((vector_size (4 * sizeof (int))));
> +  VI vi = { 0, 0, 0, 0 };
> +  VF vf = { 0.0f, 0.0f, 0.0f, 0.0f };
>  #ifdef __cplusplus
>    struct T { void foo (); };
>    int &r = a[0];
> @@ -43,6 +47,8 @@ main ()
>    static_assert (__builtin_classify_type (struct S) == 12, "");
>    static_assert (__builtin_classify_type (union U) == 13, "");
>    static_assert (__builtin_classify_type (int [2]) == 14, "");
> +  static_assert (__builtin_classify_type (VI) == 19, "");
> +  static_assert (__builtin_classify_type (VF) == 19, "");
>    static_assert (__builtin_classify_type (__typeof__ (a[0])) == 1, "");
>    static_assert (__builtin_classify_type (__typeof__ (e)) == 3, "");
>    static_assert (__builtin_classify_type (__typeof__ (b)) == 4, "");
> @@ -57,6 +63,8 @@ main ()
>    static_assert (__builtin_classify_type (__typeof__ (s)) == 12, "");
>    static_assert (__builtin_classify_type (__typeof__ (u)) == 13, "");
>    static_assert (__builtin_classify_type (__typeof__ (a)) == 14, "");
> +  static_assert (__builtin_classify_type (__typeof__ (vi)) == 19, "");
> +  static_assert (__builtin_classify_type (__typeof__ (vf)) == 19, "");
>  #ifndef __cplusplus
>    static_assert (__builtin_classify_type (a[0]) == 1, "");
>    static_assert (__builtin_classify_type (e) == 1, "");
> @@ -102,4 +110,8 @@ main ()
>      abort ();
>    if (__builtin_classify_type (a) != 5)
>      abort ();
> +  if (__builtin_classify_type (vi) != 19)
> +    abort ();
> +  if (__builtin_classify_type (vf) != 19)
> +    abort ();
>  }
> 
> 
> 	Jakub
> 
>
  
Jakub Jelinek Nov. 20, 2023, 9:41 a.m. UTC | #2
On Mon, Nov 20, 2023 at 08:03:18AM +0000, Richard Biener wrote:
> OK.  Do we have to adjust any of our documentation for this?

I've done it this way.  We don't really document the exact values
in the documentation, so I think it is sufficient like that.
Committed to trunk.

2023-11-20  Jakub Jelinek  <jakub@redhat.com>

gcc/
	* typeclass.h (enum type_class): Add vector_type_class.
	* builtins.cc (type_to_class): Return vector_type_class for
	VECTOR_TYPE.
	* doc/extend.texi (__builtin_classify_type): Mention bit-precise
	integer types and vector types.
gcc/testsuite/
	* c-c++-common/builtin-classify-type-1.c (main): Add tests for vector
	types.

--- gcc/typeclass.h.jj	2023-09-06 17:28:24.238977355 +0200
+++ gcc/typeclass.h	2023-11-10 10:50:59.519007647 +0100
@@ -38,7 +38,7 @@ enum type_class
   record_type_class, union_type_class,
   array_type_class, string_type_class,
   lang_type_class, opaque_type_class,
-  bitint_type_class
+  bitint_type_class, vector_type_class
 };
 
 #endif /* GCC_TYPECLASS_H */
--- gcc/builtins.cc.jj	2023-11-09 09:17:40.230182483 +0100
+++ gcc/builtins.cc	2023-11-10 11:19:29.669129855 +0100
@@ -1859,6 +1859,7 @@ type_to_class (tree type)
     case LANG_TYPE:	   return lang_type_class;
     case OPAQUE_TYPE:      return opaque_type_class;
     case BITINT_TYPE:	   return bitint_type_class;
+    case VECTOR_TYPE:	   return vector_type_class;
     default:		   return no_type_class;
     }
 }
--- gcc/doc/extend.texi.jj
+++ gcc/doc/extend.texi
@@ -14746,11 +14746,11 @@ The @code{__builtin_classify_type} returns a small integer with a category
 of @var{arg} argument's type, like void type, integer type, enumeral type,
 boolean type, pointer type, reference type, offset type, real type, complex
 type, function type, method type, record type, union type, array type,
-string type, etc.  When the argument is an expression, for
-backwards compatibility reason the argument is promoted like arguments
-passed to @code{...} in varargs function, so some classes are never returned
-in certain languages.  Alternatively, the argument of the built-in
-function can be a typename, such as the @code{typeof} specifier.
+string type, bit-precise integer type, vector type, etc.  When the argument
+is an expression, for backwards compatibility reason the argument is promoted
+like arguments passed to @code{...} in varargs function, so some classes are
+never returned in certain languages.  Alternatively, the argument of the
+built-in function can be a typename, such as the @code{typeof} specifier.
 
 @smallexample
 int a[2];
--- gcc/testsuite/c-c++-common/builtin-classify-type-1.c.jj	2023-09-26 09:25:30.019599039 +0200
+++ gcc/testsuite/c-c++-common/builtin-classify-type-1.c	2023-11-10 11:02:01.927776922 +0100
@@ -22,6 +22,10 @@ main ()
   const char *p = (const char *) 0;
   float f = 0.0;
   _Complex double c = 0.0;
+  typedef int VI __attribute__((vector_size (4 * sizeof (int))));
+  typedef float VF __attribute__((vector_size (4 * sizeof (int))));
+  VI vi = { 0, 0, 0, 0 };
+  VF vf = { 0.0f, 0.0f, 0.0f, 0.0f };
 #ifdef __cplusplus
   struct T { void foo (); };
   int &r = a[0];
@@ -43,6 +47,8 @@ main ()
   static_assert (__builtin_classify_type (struct S) == 12, "");
   static_assert (__builtin_classify_type (union U) == 13, "");
   static_assert (__builtin_classify_type (int [2]) == 14, "");
+  static_assert (__builtin_classify_type (VI) == 19, "");
+  static_assert (__builtin_classify_type (VF) == 19, "");
   static_assert (__builtin_classify_type (__typeof__ (a[0])) == 1, "");
   static_assert (__builtin_classify_type (__typeof__ (e)) == 3, "");
   static_assert (__builtin_classify_type (__typeof__ (b)) == 4, "");
@@ -57,6 +63,8 @@ main ()
   static_assert (__builtin_classify_type (__typeof__ (s)) == 12, "");
   static_assert (__builtin_classify_type (__typeof__ (u)) == 13, "");
   static_assert (__builtin_classify_type (__typeof__ (a)) == 14, "");
+  static_assert (__builtin_classify_type (__typeof__ (vi)) == 19, "");
+  static_assert (__builtin_classify_type (__typeof__ (vf)) == 19, "");
 #ifndef __cplusplus
   static_assert (__builtin_classify_type (a[0]) == 1, "");
   static_assert (__builtin_classify_type (e) == 1, "");
@@ -102,4 +110,8 @@ main ()
     abort ();
   if (__builtin_classify_type (a) != 5)
     abort ();
+  if (__builtin_classify_type (vi) != 19)
+    abort ();
+  if (__builtin_classify_type (vf) != 19)
+    abort ();
 }


	Jakub
  

Patch

--- gcc/typeclass.h.jj	2023-09-06 17:28:24.238977355 +0200
+++ gcc/typeclass.h	2023-11-10 10:50:59.519007647 +0100
@@ -38,7 +38,7 @@  enum type_class
   record_type_class, union_type_class,
   array_type_class, string_type_class,
   lang_type_class, opaque_type_class,
-  bitint_type_class
+  bitint_type_class, vector_type_class
 };
 
 #endif /* GCC_TYPECLASS_H */
--- gcc/builtins.cc.jj	2023-11-09 09:17:40.230182483 +0100
+++ gcc/builtins.cc	2023-11-10 11:19:29.669129855 +0100
@@ -1859,6 +1859,7 @@  type_to_class (tree type)
     case LANG_TYPE:	   return lang_type_class;
     case OPAQUE_TYPE:      return opaque_type_class;
     case BITINT_TYPE:	   return bitint_type_class;
+    case VECTOR_TYPE:	   return vector_type_class;
     default:		   return no_type_class;
     }
 }
--- gcc/testsuite/c-c++-common/builtin-classify-type-1.c.jj	2023-09-26 09:25:30.019599039 +0200
+++ gcc/testsuite/c-c++-common/builtin-classify-type-1.c	2023-11-10 11:02:01.927776922 +0100
@@ -22,6 +22,10 @@  main ()
   const char *p = (const char *) 0;
   float f = 0.0;
   _Complex double c = 0.0;
+  typedef int VI __attribute__((vector_size (4 * sizeof (int))));
+  typedef float VF __attribute__((vector_size (4 * sizeof (int))));
+  VI vi = { 0, 0, 0, 0 };
+  VF vf = { 0.0f, 0.0f, 0.0f, 0.0f };
 #ifdef __cplusplus
   struct T { void foo (); };
   int &r = a[0];
@@ -43,6 +47,8 @@  main ()
   static_assert (__builtin_classify_type (struct S) == 12, "");
   static_assert (__builtin_classify_type (union U) == 13, "");
   static_assert (__builtin_classify_type (int [2]) == 14, "");
+  static_assert (__builtin_classify_type (VI) == 19, "");
+  static_assert (__builtin_classify_type (VF) == 19, "");
   static_assert (__builtin_classify_type (__typeof__ (a[0])) == 1, "");
   static_assert (__builtin_classify_type (__typeof__ (e)) == 3, "");
   static_assert (__builtin_classify_type (__typeof__ (b)) == 4, "");
@@ -57,6 +63,8 @@  main ()
   static_assert (__builtin_classify_type (__typeof__ (s)) == 12, "");
   static_assert (__builtin_classify_type (__typeof__ (u)) == 13, "");
   static_assert (__builtin_classify_type (__typeof__ (a)) == 14, "");
+  static_assert (__builtin_classify_type (__typeof__ (vi)) == 19, "");
+  static_assert (__builtin_classify_type (__typeof__ (vf)) == 19, "");
 #ifndef __cplusplus
   static_assert (__builtin_classify_type (a[0]) == 1, "");
   static_assert (__builtin_classify_type (e) == 1, "");
@@ -102,4 +110,8 @@  main ()
     abort ();
   if (__builtin_classify_type (a) != 5)
     abort ();
+  if (__builtin_classify_type (vi) != 19)
+    abort ();
+  if (__builtin_classify_type (vf) != 19)
+    abort ();
 }