[PATCH v2 3/4] aarch64: Consolidate simd type lookup functions

[PATCH v2 3/4] aarch64: Consolidate simd type lookup functions

[Andrew Carlotti]

There were several similarly-named functions, which each built or looked up a
type using a different subset of valid modes or qualifiers.

This change combines these all into a single function, which can additionally
handle const and pointer qualifiers.

gcc/ChangeLog:

	* config/aarch64/aarch64-builtins.cc
	(aarch64_simd_builtin_std_type, aarch64_lookup_simd_builtin_type)
	(aarch64_simd_builtin_type): Combine and replace with...
	(aarch64_build_simd_builtin_type): ...this new function.
	(aarch64_init_fcmla_laneq_builtins): Update to call new function.
	(aarch64_init_simd_builtin_functions): Ditto.
	(aarch64_init_crc32_builtins): Ditto.

---

diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
index 55ad2e8b6831d6cc2b039270c8656d429347092d..6b413a36a09c7a4ac41b0fe7c414a3247580f222 100644
--- a/gcc/config/aarch64/aarch64-builtins.cc
+++ b/gcc/config/aarch64/aarch64-builtins.cc
@@ -789,79 +789,101 @@ aarch64_general_mangle_builtin_type (const_tree type)
 }
 
 static tree
-aarch64_simd_builtin_std_type (machine_mode mode,
-			       enum aarch64_type_qualifiers q)
-{
-#define QUAL_TYPE(M)  \
-  ((q == qualifier_none) ? int##M##_type_node : unsigned_int##M##_type_node);
-  switch (mode)
-    {
-    case E_QImode:
-      return QUAL_TYPE (QI);
-    case E_HImode:
-      return QUAL_TYPE (HI);
-    case E_SImode:
-      return QUAL_TYPE (SI);
-    case E_DImode:
-      return QUAL_TYPE (DI);
-    case E_TImode:
-      return QUAL_TYPE (TI);
-    case E_OImode:
-      return aarch64_simd_intOI_type_node;
-    case E_CImode:
-      return aarch64_simd_intCI_type_node;
-    case E_XImode:
-      return aarch64_simd_intXI_type_node;
-    case E_HFmode:
-      return aarch64_fp16_type_node;
-    case E_SFmode:
-      return float_type_node;
-    case E_DFmode:
-      return double_type_node;
-    case E_BFmode:
-      return aarch64_bf16_type_node;
-    default:
-      gcc_unreachable ();
-    }
-#undef QUAL_TYPE
-}
-
-static tree
-aarch64_lookup_simd_builtin_type (machine_mode mode,
-				  enum aarch64_type_qualifiers q)
+aarch64_build_simd_builtin_type (machine_mode mode,
+				 enum aarch64_type_qualifiers qualifiers)
 {
+  tree type = NULL_TREE;
   int i;
   int nelts = sizeof (aarch64_simd_types) / sizeof (aarch64_simd_types[0]);
 
-  /* Non-poly scalar modes map to standard types not in the table.  */
-  if (q != qualifier_poly && !VECTOR_MODE_P (mode))
-    return aarch64_simd_builtin_std_type (mode, q);
+  /* For pointers, we want a pointer to the basic type of the vector.  */
+  if ((qualifiers & qualifier_pointer) && VECTOR_MODE_P (mode))
+    mode = GET_MODE_INNER (mode);
 
-  for (i = 0; i < nelts; i++)
+  if ((qualifiers & qualifier_poly) || VECTOR_MODE_P (mode))
     {
-      if (aarch64_simd_types[i].mode == mode
-	  && aarch64_simd_types[i].q == q)
-	return aarch64_simd_types[i].itype;
-      if (aarch64_simd_tuple_types[i][0] != NULL_TREE)
-	for (int j = 0; j < 3; j++)
-	  if (aarch64_simd_tuple_modes[i][j] == mode
+      int q = qualifiers & (qualifier_poly | qualifier_unsigned);
+      /* Poly or vector modes map to types in the table.  */
+      for (i = 0; i < nelts; i++)
+	{
+	  if (aarch64_simd_types[i].mode == mode
 	      && aarch64_simd_types[i].q == q)
-	    return aarch64_simd_tuple_types[i][j];
+	    {
+	      type = aarch64_simd_types[i].itype;
+	      goto finished_type_lookup;
+	    }
+	  if (aarch64_simd_tuple_types[i][0] != NULL_TREE)
+	    {
+	      for (int j = 0; j < 3; j++)
+		{
+		  if (aarch64_simd_tuple_modes[i][j] == mode
+		    && aarch64_simd_types[i].q == q)
+		    {
+		      type = aarch64_simd_tuple_types[i][j];
+		      goto finished_type_lookup;
+		    }
+		}
+	    }
+	}
     }
+  else
+    {
+      /* Non-poly scalar modes map to standard types.  */
+#define QUAL_TYPE(M) ((qualifiers & qualifier_unsigned) \
+		       ? unsigned_int##M##_type_node : int##M##_type_node);
+      switch (mode)
+	{
+	case E_QImode:
+	  type = QUAL_TYPE (QI);
+	  break;
+	case E_HImode:
+	  type = QUAL_TYPE (HI);
+	  break;
+	case E_SImode:
+	  type = QUAL_TYPE (SI);
+	  break;
+	case E_DImode:
+	  type = QUAL_TYPE (DI);
+	  break;
+	case E_TImode:
+	  type = QUAL_TYPE (TI);
+	  break;
+	case E_OImode:
+	  type = aarch64_simd_intOI_type_node;
+	  break;
+	case E_CImode:
+	  type = aarch64_simd_intCI_type_node;
+	  break;
+	case E_XImode:
+	  type = aarch64_simd_intXI_type_node;
+	  break;
+	case E_HFmode:
+	  type = aarch64_fp16_type_node;
+	  break;
+	case E_SFmode:
+	  type = float_type_node;
+	  break;
+	case E_DFmode:
+	  type = double_type_node;
+	  break;
+	case E_BFmode:
+	  type = aarch64_bf16_type_node;
+	  break;
+	default:
+	  gcc_unreachable ();
+	}
+#undef QUAL_TYPE
+    }
+finished_type_lookup:
+  gcc_assert (type != NULL_TREE);
 
-  return NULL_TREE;
-}
+  /* Add qualifiers.  */
+  if (qualifiers & qualifier_const)
+    type = build_qualified_type (type, TYPE_QUAL_CONST);
+  if (qualifiers & qualifier_pointer)
+    type = build_pointer_type (type);
 
-static tree
-aarch64_simd_builtin_type (machine_mode mode,
-			   bool unsigned_p, bool poly_p)
-{
-  if (poly_p)
-    return aarch64_lookup_simd_builtin_type (mode, qualifier_poly);
-  else if (unsigned_p)
-    return aarch64_lookup_simd_builtin_type (mode, qualifier_unsigned);
-  else
-    return aarch64_lookup_simd_builtin_type (mode, qualifier_none);
+  return type;
 }
  
 static void
@@ -1110,12 +1132,12 @@ aarch64_init_fcmla_laneq_builtins (void)
     {
       aarch64_fcmla_laneq_builtin_datum* d
 	= &aarch64_fcmla_lane_builtin_data[i];
-      tree argtype = aarch64_lookup_simd_builtin_type (d->mode, qualifier_none);
+      tree argtype = aarch64_build_simd_builtin_type (d->mode, qualifier_none);
       machine_mode quadmode = GET_MODE_2XWIDER_MODE (d->mode).require ();
       tree quadtype
-	= aarch64_lookup_simd_builtin_type (quadmode, qualifier_none);
+	= aarch64_build_simd_builtin_type (quadmode, qualifier_none);
       tree lanetype
-	= aarch64_simd_builtin_std_type (SImode, qualifier_lane_pair_index);
+	= aarch64_build_simd_builtin_type (SImode, qualifier_lane_pair_index);
       tree ftype = build_function_type_list (argtype, argtype, argtype,
 					     quadtype, lanetype, NULL_TREE);
       tree attrs = aarch64_get_attributes (FLAG_FP, d->mode);
@@ -1210,23 +1232,7 @@ aarch64_init_simd_builtin_functions (bool called_from_pragma)
 	  if (qualifiers & qualifier_map_mode)
 	      op_mode = d->mode;
 
-	  /* For pointers, we want a pointer to the basic type
-	     of the vector.  */
-	  if (qualifiers & qualifier_pointer && VECTOR_MODE_P (op_mode))
-	    op_mode = GET_MODE_INNER (op_mode);
-
-	  eltype = aarch64_simd_builtin_type
-		     (op_mode,
-		      (qualifiers & qualifier_unsigned) != 0,
-		      (qualifiers & qualifier_poly) != 0);
-	  gcc_assert (eltype != NULL);
-
-	  /* Add qualifiers.  */
-	  if (qualifiers & qualifier_const)
-	    eltype = build_qualified_type (eltype, TYPE_QUAL_CONST);
-
-	  if (qualifiers & qualifier_pointer)
-	      eltype = build_pointer_type (eltype);
+	  eltype = aarch64_build_simd_builtin_type (op_mode, qualifiers);
 
 	  /* If we have reached arg_num == 0, we are at a non-void
 	     return type.  Otherwise, we are still processing
@@ -1383,13 +1389,13 @@ aarch64_init_simd_builtins (void)
 static void
 aarch64_init_crc32_builtins ()
 {
-  tree usi_type = aarch64_simd_builtin_std_type (SImode, qualifier_unsigned);
+  tree usi_type = aarch64_build_simd_builtin_type (SImode, qualifier_unsigned);
   unsigned int i = 0;
 
   for (i = 0; i < ARRAY_SIZE (aarch64_crc_builtin_data); ++i)
     {
       aarch64_crc_builtin_datum* d = &aarch64_crc_builtin_data[i];
-      tree argtype = aarch64_simd_builtin_std_type (d->mode,
+      tree argtype = aarch64_build_simd_builtin_type (d->mode,
 						    qualifier_unsigned);
       tree ftype = build_function_type_list (usi_type, usi_type, argtype, NULL_TREE);
       tree attrs = aarch64_get_attributes (FLAG_NONE, d->mode);

Re: [PATCH v2 3/4] aarch64: Consolidate simd type lookup functions

[Richard Sandiford]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> There were several similarly-named functions, which each built or looked up a
> type using a different subset of valid modes or qualifiers.
>
> This change combines these all into a single function, which can additionally
> handle const and pointer qualifiers.

I like the part about getting rid of:

static tree
aarch64_simd_builtin_type (machine_mode mode,
			   bool unsigned_p, bool poly_p)

and the flow of the new function.  However, I think it's still
slightly more readable if we keep the switch and lookup routines
separate, partly to keep down the size of the main routine and
partly to avoid the goto.

So how about:

- aarch64_simd_builtin_std_type becomes aarch64_int_or_fp_element_type
  but otherwise stays as-is

- aarch64_lookup_simd_builtin_type becomes aarch64_lookup_simd_type_in_table,
  without the:

  /* Non-poly scalar modes map to standard types not in the table.  */
  if (q != qualifier_poly && !VECTOR_MODE_P (mode))
    return aarch64_simd_builtin_std_type (mode, q);

  that your new routine handles instead.

- The new routine is called aarch64_simd_builtin_type rather than
  aarch64_build_simd_builtin_type (since the latter implies creating
  a new type).  It uses the routines:

  if ((qualifiers & qualifier_poly) || VECTOR_MODE_P (mode))
    type = aarch64_lookup_simd_type_in_table (mode, q);
  else
    type = aarch64_int_or_fp_element_type (mode, q);
  gcc_assert (type);

?

I realise this is all eye of the beholder stuff though.

Thanks,
Richard

> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-builtins.cc
> 	(aarch64_simd_builtin_std_type, aarch64_lookup_simd_builtin_type)
> 	(aarch64_simd_builtin_type): Combine and replace with...
> 	(aarch64_build_simd_builtin_type): ...this new function.
> 	(aarch64_init_fcmla_laneq_builtins): Update to call new function.
> 	(aarch64_init_simd_builtin_functions): Ditto.
> 	(aarch64_init_crc32_builtins): Ditto.
>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
> index 55ad2e8b6831d6cc2b039270c8656d429347092d..6b413a36a09c7a4ac41b0fe7c414a3247580f222 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -789,79 +789,101 @@ aarch64_general_mangle_builtin_type (const_tree type)
>  }
>  
>  static tree
> -aarch64_simd_builtin_std_type (machine_mode mode,
> -			       enum aarch64_type_qualifiers q)
> -{
> -#define QUAL_TYPE(M)  \
> -  ((q == qualifier_none) ? int##M##_type_node : unsigned_int##M##_type_node);
> -  switch (mode)
> -    {
> -    case E_QImode:
> -      return QUAL_TYPE (QI);
> -    case E_HImode:
> -      return QUAL_TYPE (HI);
> -    case E_SImode:
> -      return QUAL_TYPE (SI);
> -    case E_DImode:
> -      return QUAL_TYPE (DI);
> -    case E_TImode:
> -      return QUAL_TYPE (TI);
> -    case E_OImode:
> -      return aarch64_simd_intOI_type_node;
> -    case E_CImode:
> -      return aarch64_simd_intCI_type_node;
> -    case E_XImode:
> -      return aarch64_simd_intXI_type_node;
> -    case E_HFmode:
> -      return aarch64_fp16_type_node;
> -    case E_SFmode:
> -      return float_type_node;
> -    case E_DFmode:
> -      return double_type_node;
> -    case E_BFmode:
> -      return aarch64_bf16_type_node;
> -    default:
> -      gcc_unreachable ();
> -    }
> -#undef QUAL_TYPE
> -}
> -
> -static tree
> -aarch64_lookup_simd_builtin_type (machine_mode mode,
> -				  enum aarch64_type_qualifiers q)
> +aarch64_build_simd_builtin_type (machine_mode mode,
> +				 enum aarch64_type_qualifiers qualifiers)
>  {
> +  tree type = NULL_TREE;
>    int i;
>    int nelts = sizeof (aarch64_simd_types) / sizeof (aarch64_simd_types[0]);
>  
> -  /* Non-poly scalar modes map to standard types not in the table.  */
> -  if (q != qualifier_poly && !VECTOR_MODE_P (mode))
> -    return aarch64_simd_builtin_std_type (mode, q);
> +  /* For pointers, we want a pointer to the basic type of the vector.  */
> +  if ((qualifiers & qualifier_pointer) && VECTOR_MODE_P (mode))
> +    mode = GET_MODE_INNER (mode);
>  
> -  for (i = 0; i < nelts; i++)
> +  if ((qualifiers & qualifier_poly) || VECTOR_MODE_P (mode))
>      {
> -      if (aarch64_simd_types[i].mode == mode
> -	  && aarch64_simd_types[i].q == q)
> -	return aarch64_simd_types[i].itype;
> -      if (aarch64_simd_tuple_types[i][0] != NULL_TREE)
> -	for (int j = 0; j < 3; j++)
> -	  if (aarch64_simd_tuple_modes[i][j] == mode
> +      int q = qualifiers & (qualifier_poly | qualifier_unsigned);
> +      /* Poly or vector modes map to types in the table.  */
> +      for (i = 0; i < nelts; i++)
> +	{
> +	  if (aarch64_simd_types[i].mode == mode
>  	      && aarch64_simd_types[i].q == q)
> -	    return aarch64_simd_tuple_types[i][j];
> +	    {
> +	      type = aarch64_simd_types[i].itype;
> +	      goto finished_type_lookup;
> +	    }
> +	  if (aarch64_simd_tuple_types[i][0] != NULL_TREE)
> +	    {
> +	      for (int j = 0; j < 3; j++)
> +		{
> +		  if (aarch64_simd_tuple_modes[i][j] == mode
> +		    && aarch64_simd_types[i].q == q)
> +		    {
> +		      type = aarch64_simd_tuple_types[i][j];
> +		      goto finished_type_lookup;
> +		    }
> +		}
> +	    }
> +	}
>      }
> +  else
> +    {
> +      /* Non-poly scalar modes map to standard types.  */
> +#define QUAL_TYPE(M) ((qualifiers & qualifier_unsigned) \
> +		       ? unsigned_int##M##_type_node : int##M##_type_node);
> +      switch (mode)
> +	{
> +	case E_QImode:
> +	  type = QUAL_TYPE (QI);
> +	  break;
> +	case E_HImode:
> +	  type = QUAL_TYPE (HI);
> +	  break;
> +	case E_SImode:
> +	  type = QUAL_TYPE (SI);
> +	  break;
> +	case E_DImode:
> +	  type = QUAL_TYPE (DI);
> +	  break;
> +	case E_TImode:
> +	  type = QUAL_TYPE (TI);
> +	  break;
> +	case E_OImode:
> +	  type = aarch64_simd_intOI_type_node;
> +	  break;
> +	case E_CImode:
> +	  type = aarch64_simd_intCI_type_node;
> +	  break;
> +	case E_XImode:
> +	  type = aarch64_simd_intXI_type_node;
> +	  break;
> +	case E_HFmode:
> +	  type = aarch64_fp16_type_node;
> +	  break;
> +	case E_SFmode:
> +	  type = float_type_node;
> +	  break;
> +	case E_DFmode:
> +	  type = double_type_node;
> +	  break;
> +	case E_BFmode:
> +	  type = aarch64_bf16_type_node;
> +	  break;
> +	default:
> +	  gcc_unreachable ();
> +	}
> +#undef QUAL_TYPE
> +    }
> +finished_type_lookup:
> +  gcc_assert (type != NULL_TREE);
>  
> -  return NULL_TREE;
> -}
> +  /* Add qualifiers.  */
> +  if (qualifiers & qualifier_const)
> +    type = build_qualified_type (type, TYPE_QUAL_CONST);
> +  if (qualifiers & qualifier_pointer)
> +    type = build_pointer_type (type);
>  
> -static tree
> -aarch64_simd_builtin_type (machine_mode mode,
> -			   bool unsigned_p, bool poly_p)
> -{
> -  if (poly_p)
> -    return aarch64_lookup_simd_builtin_type (mode, qualifier_poly);
> -  else if (unsigned_p)
> -    return aarch64_lookup_simd_builtin_type (mode, qualifier_unsigned);
> -  else
> -    return aarch64_lookup_simd_builtin_type (mode, qualifier_none);
> +  return type;
>  }
>   
>  static void
> @@ -1110,12 +1132,12 @@ aarch64_init_fcmla_laneq_builtins (void)
>      {
>        aarch64_fcmla_laneq_builtin_datum* d
>  	= &aarch64_fcmla_lane_builtin_data[i];
> -      tree argtype = aarch64_lookup_simd_builtin_type (d->mode, qualifier_none);
> +      tree argtype = aarch64_build_simd_builtin_type (d->mode, qualifier_none);
>        machine_mode quadmode = GET_MODE_2XWIDER_MODE (d->mode).require ();
>        tree quadtype
> -	= aarch64_lookup_simd_builtin_type (quadmode, qualifier_none);
> +	= aarch64_build_simd_builtin_type (quadmode, qualifier_none);
>        tree lanetype
> -	= aarch64_simd_builtin_std_type (SImode, qualifier_lane_pair_index);
> +	= aarch64_build_simd_builtin_type (SImode, qualifier_lane_pair_index);
>        tree ftype = build_function_type_list (argtype, argtype, argtype,
>  					     quadtype, lanetype, NULL_TREE);
>        tree attrs = aarch64_get_attributes (FLAG_FP, d->mode);
> @@ -1210,23 +1232,7 @@ aarch64_init_simd_builtin_functions (bool called_from_pragma)
>  	  if (qualifiers & qualifier_map_mode)
>  	      op_mode = d->mode;
>  
> -	  /* For pointers, we want a pointer to the basic type
> -	     of the vector.  */
> -	  if (qualifiers & qualifier_pointer && VECTOR_MODE_P (op_mode))
> -	    op_mode = GET_MODE_INNER (op_mode);
> -
> -	  eltype = aarch64_simd_builtin_type
> -		     (op_mode,
> -		      (qualifiers & qualifier_unsigned) != 0,
> -		      (qualifiers & qualifier_poly) != 0);
> -	  gcc_assert (eltype != NULL);
> -
> -	  /* Add qualifiers.  */
> -	  if (qualifiers & qualifier_const)
> -	    eltype = build_qualified_type (eltype, TYPE_QUAL_CONST);
> -
> -	  if (qualifiers & qualifier_pointer)
> -	      eltype = build_pointer_type (eltype);
> +	  eltype = aarch64_build_simd_builtin_type (op_mode, qualifiers);
>  
>  	  /* If we have reached arg_num == 0, we are at a non-void
>  	     return type.  Otherwise, we are still processing
> @@ -1383,13 +1389,13 @@ aarch64_init_simd_builtins (void)
>  static void
>  aarch64_init_crc32_builtins ()
>  {
> -  tree usi_type = aarch64_simd_builtin_std_type (SImode, qualifier_unsigned);
> +  tree usi_type = aarch64_build_simd_builtin_type (SImode, qualifier_unsigned);
>    unsigned int i = 0;
>  
>    for (i = 0; i < ARRAY_SIZE (aarch64_crc_builtin_data); ++i)
>      {
>        aarch64_crc_builtin_datum* d = &aarch64_crc_builtin_data[i];
> -      tree argtype = aarch64_simd_builtin_std_type (d->mode,
> +      tree argtype = aarch64_build_simd_builtin_type (d->mode,
>  						    qualifier_unsigned);
>        tree ftype = build_function_type_list (usi_type, usi_type, argtype, NULL_TREE);
>        tree attrs = aarch64_get_attributes (FLAG_NONE, d->mode);

[PATCH v2.1 3/4] aarch64: Consolidate simd type lookup functions

[Andrew Carlotti]

On Wed, Jul 13, 2022 at 05:36:04PM +0100, Richard Sandiford wrote:
> I like the part about getting rid of:
> 
> static tree
> aarch64_simd_builtin_type (machine_mode mode,
> 			   bool unsigned_p, bool poly_p)
> 
> and the flow of the new function.  However, I think it's still
> slightly more readable if we keep the switch and lookup routines
> separate, partly to keep down the size of the main routine and
> partly to avoid the goto.

I agree.

> So how about:
> 
> - aarch64_simd_builtin_std_type becomes aarch64_int_or_fp_element_type
>   but otherwise stays as-is
>
> ...

I've called it aarch64_int_or_fp_type, because it's sometimes used for
an operand that doesn't represent an element of a vector.


Updated patch below.

---

There were several similarly-named functions, which each built or looked up an
operand type using a different subset of valid modes or qualifiers.

This change provides a single function to return operand types, which can
additionally handle const and pointer qualifiers. For clarity, the existing
functionality is kept in separate helper functions.

gcc/ChangeLog:

	* config/aarch64/aarch64-builtins.cc
	(aarch64_simd_builtin_std_type): Rename to...
	(aarch64_int_or_fp_type): ...this, and allow irrelevant qualifiers.
	(aarch64_lookup_simd_builtin_type): Rename to...
	(aarch64_simd_builtin_type): ...this. Add const/pointer
	support, and extract table lookup to...
	(aarch64_lookup_simd_type_in_table): ...this function.
	(aarch64_init_crc32_builtins): Update to use aarch64_simd_builtin_type.
	(aarch64_init_fcmla_laneq_builtins): Ditto.
	(aarch64_init_simd_builtin_functions): Ditto.

---


diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
index 55ad2e8b6831d6cc2b039270c8656d429347092d..cd7c2a79d9b4d67adf1d9de1f9b56eb3a0d1ee2b 100644
--- a/gcc/config/aarch64/aarch64-builtins.cc
+++ b/gcc/config/aarch64/aarch64-builtins.cc
@@ -788,12 +788,13 @@ aarch64_general_mangle_builtin_type (const_tree type)
   return NULL;
 }
 
+/* Helper function for aarch64_simd_builtin_type. */
 static tree
-aarch64_simd_builtin_std_type (machine_mode mode,
-			       enum aarch64_type_qualifiers q)
+aarch64_int_or_fp_type (machine_mode mode,
+			       enum aarch64_type_qualifiers qualifiers)
 {
-#define QUAL_TYPE(M)  \
-  ((q == qualifier_none) ? int##M##_type_node : unsigned_int##M##_type_node);
+#define QUAL_TYPE(M) ((qualifiers & qualifier_unsigned) \
+		       ? unsigned_int##M##_type_node : int##M##_type_node);
   switch (mode)
     {
     case E_QImode:
@@ -826,16 +827,14 @@ aarch64_simd_builtin_std_type (machine_mode mode,
 #undef QUAL_TYPE
 }
 
+/* Helper function for aarch64_simd_builtin_type. */
 static tree
-aarch64_lookup_simd_builtin_type (machine_mode mode,
-				  enum aarch64_type_qualifiers q)
+aarch64_lookup_simd_type_in_table (machine_mode mode,
+				   enum aarch64_type_qualifiers qualifiers)
 {
   int i;
   int nelts = sizeof (aarch64_simd_types) / sizeof (aarch64_simd_types[0]);
-
-  /* Non-poly scalar modes map to standard types not in the table.  */
-  if (q != qualifier_poly && !VECTOR_MODE_P (mode))
-    return aarch64_simd_builtin_std_type (mode, q);
+  int q = qualifiers & (qualifier_poly | qualifier_unsigned);
 
   for (i = 0; i < nelts; i++)
     {
@@ -852,16 +851,32 @@ aarch64_lookup_simd_builtin_type (machine_mode mode,
   return NULL_TREE;
 }
 
+/* Return a type for an operand with specified mode and qualifiers. */
 static tree
 aarch64_simd_builtin_type (machine_mode mode,
-			   bool unsigned_p, bool poly_p)
+			   enum aarch64_type_qualifiers qualifiers)
 {
-  if (poly_p)
-    return aarch64_lookup_simd_builtin_type (mode, qualifier_poly);
-  else if (unsigned_p)
-    return aarch64_lookup_simd_builtin_type (mode, qualifier_unsigned);
+  tree type = NULL_TREE;
+
+  /* For pointers, we want a pointer to the basic type of the vector.  */
+  if ((qualifiers & qualifier_pointer) && VECTOR_MODE_P (mode))
+    mode = GET_MODE_INNER (mode);
+
+  /* Non-poly scalar modes map to standard types not in the table.  */
+  if ((qualifiers & qualifier_poly) || VECTOR_MODE_P (mode))
+    type = aarch64_lookup_simd_type_in_table (mode, qualifiers);
   else
-    return aarch64_lookup_simd_builtin_type (mode, qualifier_none);
+    type = aarch64_int_or_fp_type (mode, qualifiers);
+
+  gcc_assert (type != NULL_TREE);
+
+  /* Add qualifiers.  */
+  if (qualifiers & qualifier_const)
+    type = build_qualified_type (type, TYPE_QUAL_CONST);
+  if (qualifiers & qualifier_pointer)
+    type = build_pointer_type (type);
+
+  return type;
 }
  
 static void
@@ -1110,12 +1125,11 @@ aarch64_init_fcmla_laneq_builtins (void)
     {
       aarch64_fcmla_laneq_builtin_datum* d
 	= &aarch64_fcmla_lane_builtin_data[i];
-      tree argtype = aarch64_lookup_simd_builtin_type (d->mode, qualifier_none);
+      tree argtype = aarch64_simd_builtin_type (d->mode, qualifier_none);
       machine_mode quadmode = GET_MODE_2XWIDER_MODE (d->mode).require ();
-      tree quadtype
-	= aarch64_lookup_simd_builtin_type (quadmode, qualifier_none);
+      tree quadtype = aarch64_simd_builtin_type (quadmode, qualifier_none);
       tree lanetype
-	= aarch64_simd_builtin_std_type (SImode, qualifier_lane_pair_index);
+	= aarch64_simd_builtin_type (SImode, qualifier_lane_pair_index);
       tree ftype = build_function_type_list (argtype, argtype, argtype,
 					     quadtype, lanetype, NULL_TREE);
       tree attrs = aarch64_get_attributes (FLAG_FP, d->mode);
@@ -1210,23 +1224,7 @@ aarch64_init_simd_builtin_functions (bool called_from_pragma)
 	  if (qualifiers & qualifier_map_mode)
 	      op_mode = d->mode;
 
-	  /* For pointers, we want a pointer to the basic type
-	     of the vector.  */
-	  if (qualifiers & qualifier_pointer && VECTOR_MODE_P (op_mode))
-	    op_mode = GET_MODE_INNER (op_mode);
-
-	  eltype = aarch64_simd_builtin_type
-		     (op_mode,
-		      (qualifiers & qualifier_unsigned) != 0,
-		      (qualifiers & qualifier_poly) != 0);
-	  gcc_assert (eltype != NULL);
-
-	  /* Add qualifiers.  */
-	  if (qualifiers & qualifier_const)
-	    eltype = build_qualified_type (eltype, TYPE_QUAL_CONST);
-
-	  if (qualifiers & qualifier_pointer)
-	      eltype = build_pointer_type (eltype);
+	  eltype = aarch64_simd_builtin_type (op_mode, qualifiers);
 
 	  /* If we have reached arg_num == 0, we are at a non-void
 	     return type.  Otherwise, we are still processing
@@ -1383,13 +1381,13 @@ aarch64_init_simd_builtins (void)
 static void
 aarch64_init_crc32_builtins ()
 {
-  tree usi_type = aarch64_simd_builtin_std_type (SImode, qualifier_unsigned);
+  tree usi_type = aarch64_simd_builtin_type (SImode, qualifier_unsigned);
   unsigned int i = 0;
 
   for (i = 0; i < ARRAY_SIZE (aarch64_crc_builtin_data); ++i)
     {
       aarch64_crc_builtin_datum* d = &aarch64_crc_builtin_data[i];
-      tree argtype = aarch64_simd_builtin_std_type (d->mode,
+      tree argtype = aarch64_simd_builtin_type (d->mode,
 						    qualifier_unsigned);
       tree ftype = build_function_type_list (usi_type, usi_type, argtype, NULL_TREE);
       tree attrs = aarch64_get_attributes (FLAG_NONE, d->mode);

Re: [PATCH v2.1 3/4] aarch64: Consolidate simd type lookup functions

[Richard Sandiford]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> On Wed, Jul 13, 2022 at 05:36:04PM +0100, Richard Sandiford wrote:
>> I like the part about getting rid of:
>> 
>> static tree
>> aarch64_simd_builtin_type (machine_mode mode,
>> 			   bool unsigned_p, bool poly_p)
>> 
>> and the flow of the new function.  However, I think it's still
>> slightly more readable if we keep the switch and lookup routines
>> separate, partly to keep down the size of the main routine and
>> partly to avoid the goto.
>
> I agree.
>
>> So how about:
>> 
>> - aarch64_simd_builtin_std_type becomes aarch64_int_or_fp_element_type
>>   but otherwise stays as-is
>>
>> ...
>
> I've called it aarch64_int_or_fp_type, because it's sometimes used for
> an operand that doesn't represent an element of a vector.
>
>
> Updated patch below.
>
> ---
>
> There were several similarly-named functions, which each built or looked up an
> operand type using a different subset of valid modes or qualifiers.
>
> This change provides a single function to return operand types, which can
> additionally handle const and pointer qualifiers. For clarity, the existing
> functionality is kept in separate helper functions.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-builtins.cc
> 	(aarch64_simd_builtin_std_type): Rename to...
> 	(aarch64_int_or_fp_type): ...this, and allow irrelevant qualifiers.
> 	(aarch64_lookup_simd_builtin_type): Rename to...
> 	(aarch64_simd_builtin_type): ...this. Add const/pointer
> 	support, and extract table lookup to...
> 	(aarch64_lookup_simd_type_in_table): ...this function.
> 	(aarch64_init_crc32_builtins): Update to use aarch64_simd_builtin_type.
> 	(aarch64_init_fcmla_laneq_builtins): Ditto.
> 	(aarch64_init_simd_builtin_functions): Ditto.

LGTM, thanks.  OK for trunk with a couple of minor formatting fixes:

> diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
> index 55ad2e8b6831d6cc2b039270c8656d429347092d..cd7c2a79d9b4d67adf1d9de1f9b56eb3a0d1ee2b 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -788,12 +788,13 @@ aarch64_general_mangle_builtin_type (const_tree type)
>    return NULL;
>  }
>  
> +/* Helper function for aarch64_simd_builtin_type. */
>  static tree
> -aarch64_simd_builtin_std_type (machine_mode mode,
> -			       enum aarch64_type_qualifiers q)
> +aarch64_int_or_fp_type (machine_mode mode,
> +			       enum aarch64_type_qualifiers qualifiers)

This line should be reindented so that the arguments continue to line up.

>  {
> -#define QUAL_TYPE(M)  \
> -  ((q == qualifier_none) ? int##M##_type_node : unsigned_int##M##_type_node);
> +#define QUAL_TYPE(M) ((qualifiers & qualifier_unsigned) \
> +		       ? unsigned_int##M##_type_node : int##M##_type_node);
>    switch (mode)
>      {
>      case E_QImode:
> [...]
> @@ -1383,13 +1381,13 @@ aarch64_init_simd_builtins (void)
>  static void
>  aarch64_init_crc32_builtins ()
>  {
> -  tree usi_type = aarch64_simd_builtin_std_type (SImode, qualifier_unsigned);
> +  tree usi_type = aarch64_simd_builtin_type (SImode, qualifier_unsigned);
>    unsigned int i = 0;
>  
>    for (i = 0; i < ARRAY_SIZE (aarch64_crc_builtin_data); ++i)
>      {
>        aarch64_crc_builtin_datum* d = &aarch64_crc_builtin_data[i];
> -      tree argtype = aarch64_simd_builtin_std_type (d->mode,
> +      tree argtype = aarch64_simd_builtin_type (d->mode,
>  						    qualifier_unsigned);

Same here.

Richard

>        tree ftype = build_function_type_list (usi_type, usi_type, argtype, NULL_TREE);
>        tree attrs = aarch64_get_attributes (FLAG_NONE, d->mode);