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
