On 16/09/2024 10:38, Christophe Lyon wrote:
> From: Alfie Richards <alfie.richa...@arm.com>
>
> This patch adds the load_extending and store_truncating function bases
> for MVE intrinsics.
>
> The constructors have parameters describing the memory element
> type/width which is part of the function base name (e.g. "h" in
> vldrhq).
>
> 2024-09-11 Alfie Richards <alfie.richa...@arm.com>
>
> gcc/
>
> * config/arm/arm-mve-builtins-functions.h
> (load_extending): New class.
> (store_truncating): New class.
> * config/arm/arm-protos.h (arm_mve_data_mode): New helper function.
> * config/arm/arm.cc (arm_mve_data_mode): New helper function.
This patch is technically ok, but there are some formatting issues that make
the code layout slightly confusing and hard to read:
+ return arm_mve_data_mode (GET_MODE_INNER (mem_mode),
+ GET_MODE_NUNITS (reg_mode))
+ .require ();
The stray ".require ();" on its own looks strange given the indentation. Your
line is short enough that you can write
+ return arm_mve_data_mode (GET_MODE_INNER (mem_mode),
+ GET_MODE_NUNITS (reg_mode)).require ();
+ unsigned int element_bits = GET_MODE_BITSIZE (
+ (fi.type_suffix (0).integer_p
+ ? m_to_int_mode
+ : m_to_float_mode.require ()));
Here you should put "= GET_MODE_BITSIZE (" on the following line, then indent
the arguments to the opening paren of the function arguments:
+ unsigned int element_bits
+ = GET_MODE_BITSIZE (fi.type_suffix (0).integer_p
+ ? m_to_int_mode
+ : m_to_float_mode.require ());
And you can then lose the extra level of parenthesis.
+ return arm_mve_data_mode (
+ (fi.type_suffix (0).integer_p
+ ? m_to_int_mode
+ : m_to_float_mode.require ()),
+ nunits)
+ .require ();
In this case I'd split the selection operation into a separate statement,
giving (if I've got the type correct):
+ scalar_mode mode = (fi.type_suffix (0).integer_p
+ ? m_to_int_mode
+ : m_to_float_mode.require ());
+ return arm_mve_data_mode (mode, nunits).require ();
OK with those changes.
R.
> ---
> gcc/config/arm/arm-mve-builtins-functions.h | 106 ++++++++++++++++++++
> gcc/config/arm/arm-protos.h | 3 +
> gcc/config/arm/arm.cc | 15 +++
> 3 files changed, 124 insertions(+)
>
> diff --git a/gcc/config/arm/arm-mve-builtins-functions.h
> b/gcc/config/arm/arm-mve-builtins-functions.h
> index ac2a731bff4..e47bc69936e 100644
> --- a/gcc/config/arm/arm-mve-builtins-functions.h
> +++ b/gcc/config/arm/arm-mve-builtins-functions.h
> @@ -20,6 +20,8 @@
> #ifndef GCC_ARM_MVE_BUILTINS_FUNCTIONS_H
> #define GCC_ARM_MVE_BUILTINS_FUNCTIONS_H
>
> +#include "arm-protos.h"
> +
> namespace arm_mve {
>
> /* Wrap T, which is derived from function_base, and indicate that the
> @@ -1024,6 +1026,110 @@ public:
> }
> };
>
> +/* A function_base that loads elements from memory and extends them
> + to a wider element. The memory element type is a fixed part of
> + the function base name. */
> +class load_extending : public function_base
> +{
> +public:
> + CONSTEXPR load_extending (type_suffix_index signed_memory_type,
> + type_suffix_index unsigned_memory_type,
> + type_suffix_index float_memory_type)
> + : m_signed_memory_type (signed_memory_type),
> + m_unsigned_memory_type (unsigned_memory_type),
> + m_float_memory_type (float_memory_type)
> + {}
> + CONSTEXPR load_extending (type_suffix_index signed_memory_type,
> + type_suffix_index unsigned_memory_type)
> + : m_signed_memory_type (signed_memory_type),
> + m_unsigned_memory_type (unsigned_memory_type),
> + m_float_memory_type (NUM_TYPE_SUFFIXES)
> + {}
> +
> + unsigned int call_properties (const function_instance &) const override
> + {
> + return CP_READ_MEMORY;
> + }
> +
> + tree memory_scalar_type (const function_instance &fi) const override
> + {
> + type_suffix_index memory_type_suffix
> + = (fi.type_suffix (0).integer_p
> + ? (fi.type_suffix (0).unsigned_p
> + ? m_unsigned_memory_type
> + : m_signed_memory_type)
> + : m_float_memory_type);
> + return scalar_types[type_suffixes[memory_type_suffix].vector_type];
> + }
> +
> + machine_mode memory_vector_mode (const function_instance &fi) const
> override
> + {
> + type_suffix_index memory_type_suffix
> + = (fi.type_suffix (0).integer_p
> + ? (fi.type_suffix (0).unsigned_p
> + ? m_unsigned_memory_type
> + : m_signed_memory_type)
> + : m_float_memory_type);
> + machine_mode mem_mode = type_suffixes[memory_type_suffix].vector_mode;
> + machine_mode reg_mode = fi.vector_mode (0);
> +
> + return arm_mve_data_mode (GET_MODE_INNER (mem_mode),
> + GET_MODE_NUNITS (reg_mode))
> + .require ();
> + }
> +
> + /* The type of the memory elements. This is part of the function base
> + name rather than a true type suffix. */
> + type_suffix_index m_signed_memory_type;
> + type_suffix_index m_unsigned_memory_type;
> + type_suffix_index m_float_memory_type;
> +};
> +
> +/* A function_base that truncates vector elements and stores them to memory.
> + The memory element width is a fixed part of the function base name. */
> +class store_truncating : public function_base
> +{
> +public:
> + CONSTEXPR store_truncating (scalar_mode to_int_mode,
> + opt_scalar_mode to_float_mode)
> + : m_to_int_mode (to_int_mode), m_to_float_mode (to_float_mode)
> + {}
> +
> + unsigned int call_properties (const function_instance &) const override
> + {
> + return CP_WRITE_MEMORY;
> + }
> +
> + tree memory_scalar_type (const function_instance &fi) const override
> + {
> + /* In truncating stores, the signedness of the memory element is defined
> + to be the same as the signedness of the vector element. The
> signedness
> + doesn't make any difference to the behavior of the function. */
> + type_class_index tclass = fi.type_suffix (0).tclass;
> + unsigned int element_bits = GET_MODE_BITSIZE (
> + (fi.type_suffix (0).integer_p
> + ? m_to_int_mode
> + : m_to_float_mode.require ()));
> + type_suffix_index suffix = find_type_suffix (tclass, element_bits);
> + return scalar_types[type_suffixes[suffix].vector_type];
> + }
> +
> + machine_mode memory_vector_mode (const function_instance &fi) const
> override
> + {
> + poly_uint64 nunits = GET_MODE_NUNITS (fi.vector_mode (0));
> + return arm_mve_data_mode (
> + (fi.type_suffix (0).integer_p
> + ? m_to_int_mode
> + : m_to_float_mode.require ()),
> + nunits)
> + .require ();
> + }
> +
> + /* The mode of a single memory element. */
> + scalar_mode m_to_int_mode;
> + opt_scalar_mode m_to_float_mode;
> +};
> +
> } /* end namespace arm_mve */
>
> /* Declare the global function base NAME, creating it from an instance
> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
> index 50cae2b513a..2327f2cfe4e 100644
> --- a/gcc/config/arm/arm-protos.h
> +++ b/gcc/config/arm/arm-protos.h
> @@ -615,4 +615,7 @@ void arm_initialize_isa (sbitmap, const enum isa_feature
> *);
> const char * arm_gen_far_branch (rtx *, int, const char * , const char *);
>
> bool arm_mve_immediate_check(rtx, machine_mode, bool);
> +
> +opt_machine_mode arm_mve_data_mode (scalar_mode, poly_uint64);
> +
> #endif /* ! GCC_ARM_PROTOS_H */
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index de34e9867e6..41c4a525613 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -76,6 +76,7 @@
> #include "opts.h"
> #include "aarch-common.h"
> #include "aarch-common-protos.h"
> +#include "machmode.h"
>
> /* This file should be included last. */
> #include "target-def.h"
> @@ -36104,4 +36105,18 @@ arm_output_load_tpidr (rtx dst, bool pred_p)
> return "";
> }
>
> +/* Return the MVE vector mode that has NUNITS elements of mode INNER_MODE.
> */
> +opt_machine_mode
> +arm_mve_data_mode (scalar_mode inner_mode, poly_uint64 nunits)
> +{
> + enum mode_class mclass
> + = (SCALAR_FLOAT_MODE_P (inner_mode) ? MODE_VECTOR_FLOAT :
> MODE_VECTOR_INT);
> + machine_mode mode;
> + FOR_EACH_MODE_IN_CLASS (mode, mclass)
> + if (inner_mode == GET_MODE_INNER (mode)
> + && known_eq (nunits, GET_MODE_NUNITS (mode)))
> + return mode;
> + return opt_machine_mode ();
> +}
> +
> #include "gt-arm.h"
