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

[Richard Sandiford <richard.sandiford@arm.com>]

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);