[v2] arm: [MVE intrinsics] Avoid warnings when floating-point is not supported [PR 117814]

  If the target does not support floating-point, we register FP vector
types as 'void' (see register_vector_type).

The leads to warnings about 'pure attribute on function returning
void' when we declare the various load intrinsics because their
call_properties say CP_READ_MEMORY (thus giving them the 'pure'
attribute), but their return type is void.

To avoid such warnings, declare floating-point scalar and vector types
even if the target does not have an FPU.

In arm-mve-builtins.cc (register_builtin_types, register_vector_type,
register_builtin_tuple_types), this means simply removing the early
exits.  However, for this to work, we need to update
arm_vector_mode_supported_p, so that vector floating-point types are
always defined, and __fp16 must always be registered by
arm_init_fp16_builtins (as it is the base type for vectors of
float16_t.  Another side effect is that the declaration of float16_t
and float32_t typedefs is now unconditional

The two new tests verify that:
- we emit an error if the code tries to use a floating-point
  intrinsics and the target does not have the floating-point extension
- we emit the expected code when activating the floating-point
  expected via a pragma

gcc/ChangeLog:

	PR target/117814
	* config/arm/arm-builtins.cc (arm_init_fp16_builtins): Always
	register __fp16 type.
	* config/arm/arm-mve-builtins.cc (register_vector_type): Remove
	special handling when TARGET_HAVE_MVE_FLOAT is false.
	(register_builtin_tuple_types): Likewise.
	* config/arm/arm.cc (arm_vector_mode_supported_p): Accept
	floating-point vector modes even if TARGET_HAVE_MVE_FLOAT is
	false.
	* config/arm/arm_mve_types.h (float16_t, float32_t): Define
	unconditionally.

gcc/testsuite/ChangeLog:

	PR target/117814
	* gcc.target/arm/mve/intrinsics/pr117814-2.c: New test.
	* gcc.target/arm/mve/intrinsics/pr117814.c: New test.
---
 gcc/config/arm/arm-builtins.cc                |  5 ++--
 gcc/config/arm/arm-mve-builtins.cc            | 24 ++--------------
 gcc/config/arm/arm.cc                         |  6 +---
 gcc/config/arm/arm_mve_types.h                |  2 --
 .../arm/mve/intrinsics/pr117814-2.c           | 28 +++++++++++++++++++
 .../gcc.target/arm/mve/intrinsics/pr117814.c  | 19 +++++++++++++
 6 files changed, 52 insertions(+), 32 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814-2.c
 create mode 100644 gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814.c
  

On 02/12/2024 21:21, Christophe Lyon wrote:
> If the target does not support floating-point, we register FP vector
> types as 'void' (see register_vector_type).
> 
> The leads to warnings about 'pure attribute on function returning
> void' when we declare the various load intrinsics because their
> call_properties say CP_READ_MEMORY (thus giving them the 'pure'
> attribute), but their return type is void.
> 
> To avoid such warnings, declare floating-point scalar and vector types
> even if the target does not have an FPU.
> 
> In arm-mve-builtins.cc (register_builtin_types, register_vector_type,
> register_builtin_tuple_types), this means simply removing the early
> exits.  However, for this to work, we need to update
> arm_vector_mode_supported_p, so that vector floating-point types are
> always defined, and __fp16 must always be registered by
> arm_init_fp16_builtins (as it is the base type for vectors of
> float16_t.  Another side effect is that the declaration of float16_t
> and float32_t typedefs is now unconditional
> 
> The two new tests verify that:
> - we emit an error if the code tries to use a floating-point
>   intrinsics and the target does not have the floating-point extension
> - we emit the expected code when activating the floating-point
>   expected via a pragma
> 
> gcc/ChangeLog:
> 
> 	PR target/117814
> 	* config/arm/arm-builtins.cc (arm_init_fp16_builtins): Always
> 	register __fp16 type.
> 	* config/arm/arm-mve-builtins.cc (register_vector_type): Remove
> 	special handling when TARGET_HAVE_MVE_FLOAT is false.
> 	(register_builtin_tuple_types): Likewise.
> 	* config/arm/arm.cc (arm_vector_mode_supported_p): Accept
> 	floating-point vector modes even if TARGET_HAVE_MVE_FLOAT is
> 	false.
> 	* config/arm/arm_mve_types.h (float16_t, float32_t): Define
> 	unconditionally.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/117814
> 	* gcc.target/arm/mve/intrinsics/pr117814-2.c: New test.
> 	* gcc.target/arm/mve/intrinsics/pr117814.c: New test.


The manual says:

@opindex mfp16-format
@item -mfp16-format=@var{name}
Specify the format of the @code{__fp16} half-precision floating-point type.
Permissible names are @samp{none}, @samp{ieee}, and @samp{alternative};
the default is @samp{none}, in which case the @code{__fp16} type is not
defined.  @xref{Half-Precision}, for more information.

So I think, as stands, this is incompatible with your patch.  However, I think this is something we should perhaps fix by changing that default now.  The minimum architecture we now support is v4, so we always have ldrh, making

  /* __fp16 support currently assumes the core has ldrh.  */
  if (!arm_arch4 && arm_fp16_format != ARM_FP16_FORMAT_NONE)
    sorry ("%<__fp16%> and no ldrh");

redundant.

That does mean that some people will need to explicitly use -mfp16-format=none if they want their code to be portable to both ABI variants, but I think that's very much a niche case.

I'd be content if it were necessary for arm_mve.h to be declared incompatible with -mfp16-format being set to none (or even alternative) provided this is documented.


R.

> ---
>  gcc/config/arm/arm-builtins.cc                |  5 ++--
>  gcc/config/arm/arm-mve-builtins.cc            | 24 ++--------------
>  gcc/config/arm/arm.cc                         |  6 +---
>  gcc/config/arm/arm_mve_types.h                |  2 --
>  .../arm/mve/intrinsics/pr117814-2.c           | 28 +++++++++++++++++++
>  .../gcc.target/arm/mve/intrinsics/pr117814.c  | 19 +++++++++++++
>  6 files changed, 52 insertions(+), 32 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814-2.c
>  create mode 100644 gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814.c
> 
> diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
> index 01bdbbf943d..71b78fee55b 100644
> --- a/gcc/config/arm/arm-builtins.cc
> +++ b/gcc/config/arm/arm-builtins.cc
> @@ -2443,9 +2443,8 @@ arm_init_fp16_builtins (void)
>    arm_fp16_type_node = make_node (REAL_TYPE);
>    TYPE_PRECISION (arm_fp16_type_node) = GET_MODE_PRECISION (HFmode);
>    layout_type (arm_fp16_type_node);
> -  if (arm_fp16_format)
> -    (*lang_hooks.types.register_builtin_type) (arm_fp16_type_node,
> -					       "__fp16");
> +  (*lang_hooks.types.register_builtin_type) (arm_fp16_type_node,
> +					     "__fp16");
>  }
>  
>  void
> diff --git a/gcc/config/arm/arm-mve-builtins.cc b/gcc/config/arm/arm-mve-builtins.cc
> index 30b103ec086..25c1b442a28 100644
> --- a/gcc/config/arm/arm-mve-builtins.cc
> +++ b/gcc/config/arm/arm-mve-builtins.cc
> @@ -410,8 +410,6 @@ register_builtin_types ()
>  #include "arm-mve-builtins.def"
>    for (unsigned int i = 0; i < NUM_VECTOR_TYPES; ++i)
>      {
> -      if (vector_types[i].requires_float && !TARGET_HAVE_MVE_FLOAT)
> -	continue;
>        tree eltype = scalar_types[i];
>        tree vectype;
>        if (eltype == boolean_type_node)
> @@ -433,18 +431,6 @@ register_builtin_types ()
>  static void
>  register_vector_type (vector_type_index type)
>  {
> -
> -  /* If the target does not have the mve.fp extension, but the type requires
> -     it, then it needs to be assigned a non-dummy type so that functions
> -     with those types in their signature can be registered.  This allows for
> -     diagnostics about the missing extension, rather than about a missing
> -     function definition.  */
> -  if (vector_types[type].requires_float && !TARGET_HAVE_MVE_FLOAT)
> -    {
> -      acle_vector_types[0][type] = void_type_node;
> -      return;
> -    }
> -
>    tree vectype = abi_vector_types[type];
>    tree id = get_identifier (vector_types[type].acle_name);
>    tree decl = build_decl (input_location, TYPE_DECL, id, vectype);
> @@ -512,17 +498,11 @@ register_builtin_tuple_types (vector_type_index type)
>  {
>    const vector_type_info* info = &vector_types[type];
>  
> -  /* If the target does not have the mve.fp extension, but the type requires
> -     it, then it needs to be assigned a non-dummy type so that functions
> -     with those types in their signature can be registered.  This allows for
> -     diagnostics about the missing extension, rather than about a missing
> -     function definition.  */
> -  if (scalar_types[type] == boolean_type_node
> -      || (info->requires_float && !TARGET_HAVE_MVE_FLOAT))
> +  if (scalar_types[type] == boolean_type_node)
>      {
>        for (unsigned int num_vectors = 2; num_vectors <= 4; num_vectors += 2)
>  	acle_vector_types[num_vectors - 1][type] = void_type_node;
> -    return;
> +      return;
>      }
>  
>    const char *vector_type_name = info->acle_name;
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index 04028dbc3c3..500c8fdefc3 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -29791,11 +29791,7 @@ arm_vector_mode_supported_p (machine_mode mode)
>      return true;
>  
>    if (TARGET_HAVE_MVE
> -      && (VALID_MVE_SI_MODE (mode) || VALID_MVE_PRED_MODE (mode)))
> -    return true;
> -
> -  if (TARGET_HAVE_MVE_FLOAT
> -      && (mode == V2DFmode || mode == V4SFmode || mode == V8HFmode))
> +      && (VALID_MVE_MODE (mode) || VALID_MVE_PRED_MODE (mode)))
>      return true;
>  
>    return false;
> diff --git a/gcc/config/arm/arm_mve_types.h b/gcc/config/arm/arm_mve_types.h
> index f549f881b49..003e7e51d96 100644
> --- a/gcc/config/arm/arm_mve_types.h
> +++ b/gcc/config/arm/arm_mve_types.h
> @@ -22,10 +22,8 @@
>  #ifndef _GCC_ARM_MVE_TYPES_H
>  #define _GCC_ARM_MVE_TYPES_H
>  
> -#if (__ARM_FEATURE_MVE & 2) /* MVE Floating point.  */
>  typedef __fp16 float16_t;
>  typedef float float32_t;
> -#endif
>  
>  #pragma GCC arm "arm_mve_types.h"
>  
> diff --git a/gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814-2.c b/gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814-2.c
> new file mode 100644
> index 00000000000..ca8d414ef92
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814-2.c
> @@ -0,0 +1,28 @@
> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> +/* { dg-add-options arm_v8_1m_mve } */
> +/* { dg-additional-options "-O2" } */
> +/* { dg-final { check-function-bodies "**" "" } } */
> +
> +#include "arm_mve.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#pragma GCC target ("arch=armv8.1-m.main+mve.fp")
> +
> +/*
> +**foo1:
> +**	...
> +**	vldrh.16	q[0-9]+, \[(?:ip|fp|r[0-9]+)\](?:	@.*|)
> +**	...
> +*/
> +float16x8_t
> +foo1 (float16_t const *base)
> +{
> +  return vld1q_f16 (base);
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> diff --git a/gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814.c b/gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814.c
> new file mode 100644
> index 00000000000..5038bcb3dba
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/mve/intrinsics/pr117814.c
> @@ -0,0 +1,19 @@
> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> +/* { dg-add-options arm_v8_1m_mve } */
> +/* { dg-additional-options "-O2" } */
> +
> +#include "arm_mve.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +float16x8_t
> +foo (float16_t const *base)
> +{
> +  return vld1q_f16 (base); /* { dg-error {ACLE function 'vld1q_f16' requires ISA extension 'mve.fp'} } */
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif