Andrew Carlotti <andrew.carlo...@arm.com> writes:
> [...]
> @@ -204,6 +207,18 @@ static constexpr aarch64_processor_info all_cores[] =
> {NULL, aarch64_no_cpu, aarch64_no_arch, 0}
> };
>
> +/* Return the set of feature flags that are required to be enabled when the
> + features in FLAGS are enabled. */
> +
> +aarch64_feature_flags
> +aarch64_get_required_features (aarch64_feature_flags flags)
> +{
> + const struct aarch64_extension_info *opt;
> + for (opt = all_extensions; opt->name != NULL; opt++)
> + if (flags & opt->flag_canonical)
> + flags |= opt->flags_required;
> + return flags;
> +}
For the record, the transitive closure is available at compile time
using aarch64-features-deps.h, so we could have required clients of
aarch64_target_switcher to use that. But I agree that this approach
is simpler.
> /* Print a list of CANDIDATES for an argument, and try to suggest a specific
> close match. */
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc
> b/gcc/config/aarch64/aarch64-builtins.cc
> index
> 128cc365d3d585e01cb69668f285318ee56a36fc..5174fb1daefee2d73a5098e0de1cca73dc103416
> 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -1877,23 +1877,31 @@ aarch64_scalar_builtin_type_p (aarch64_simd_type t)
> return (t == Poly8_t || t == Poly16_t || t == Poly64_t || t == Poly128_t);
> }
>
> -/* Enable AARCH64_FL_* flags EXTRA_FLAGS on top of the base Advanced SIMD
> - set. */
> -aarch64_simd_switcher::aarch64_simd_switcher (aarch64_feature_flags
> extra_flags)
> +/* Temporarily set FLAGS as the enabled target features. */
> +aarch64_target_switcher::aarch64_target_switcher (aarch64_feature_flags
> flags)
> : m_old_asm_isa_flags (aarch64_asm_isa_flags),
> - m_old_general_regs_only (TARGET_GENERAL_REGS_ONLY)
> + m_old_general_regs_only (TARGET_GENERAL_REGS_ONLY),
> + m_old_target_pragma (current_target_pragma)
> {
> + /* Include all dependencies. */
> + flags = aarch64_get_required_features (flags);
> +
> /* Changing the ISA flags should be enough here. We shouldn't need to
> pay the compile-time cost of a full target switch. */
> - global_options.x_target_flags &= ~MASK_GENERAL_REGS_ONLY;
> - aarch64_set_asm_isa_flags (AARCH64_FL_FP | AARCH64_FL_SIMD | extra_flags);
> + if (flags & AARCH64_FL_FP)
> + global_options.x_target_flags &= ~MASK_GENERAL_REGS_ONLY;
> + aarch64_set_asm_isa_flags (flags);
In the earlier review I'd suggested keeping aarch64_simd_(target_)switcher
as a separate class, with aarch64_target_switcher being a new base class
that handles the pragma.
This patch seems to be more in the direction of treating
aarch64_target_switcher as a one-stop shop that works out what to do
based on the flags. I can see the attraction of that, but I think
we should then fold sve_(target_)switcher into it too, for consistency.
Thanks,
Richard
> +
> + /* Target pragmas are irrelevant when defining intrinsics artificially. */
> + current_target_pragma = NULL_TREE;
> }
>
> -aarch64_simd_switcher::~aarch64_simd_switcher ()
> +aarch64_target_switcher::~aarch64_target_switcher ()
> {
> if (m_old_general_regs_only)
> global_options.x_target_flags |= MASK_GENERAL_REGS_ONLY;
> aarch64_set_asm_isa_flags (m_old_asm_isa_flags);
> + current_target_pragma = m_old_target_pragma;
> }
>
> /* Implement #pragma GCC aarch64 "arm_neon.h".
> @@ -1903,7 +1911,7 @@ aarch64_simd_switcher::~aarch64_simd_switcher ()
> void
> handle_arm_neon_h (void)
> {
> - aarch64_simd_switcher simd;
> + aarch64_target_switcher switcher (AARCH64_FL_SIMD);
>
> /* Register the AdvSIMD vector tuple types. */
> for (unsigned int i = 0; i < ARM_NEON_H_TYPES_LAST; i++)
> @@ -2353,6 +2361,8 @@ aarch64_init_data_intrinsics (void)
> void
> handle_arm_acle_h (void)
> {
> + aarch64_target_switcher switcher;
> +
> aarch64_init_ls64_builtins ();
> aarch64_init_tme_builtins ();
> aarch64_init_memtag_builtins ();
> @@ -2446,7 +2456,7 @@ aarch64_general_init_builtins (void)
> aarch64_init_bf16_types ();
>
> {
> - aarch64_simd_switcher simd;
> + aarch64_target_switcher switcher (AARCH64_FL_SIMD);
> aarch64_init_simd_builtins ();
> }
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index
> 4235f4a0ca51af49c2852a420f1056727b24f345..3a809d10fa8ce2340672c4eb38168260f2c7d4e0
> 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -733,15 +733,16 @@ const unsigned int AARCH64_BUILTIN_CLASS = (1 <<
> AARCH64_BUILTIN_SHIFT) - 1;
>
> /* RAII class for enabling enough features to define built-in types
> and implement the arm_neon.h pragma. */
> -class aarch64_simd_switcher
> +class aarch64_target_switcher
> {
> public:
> - aarch64_simd_switcher (aarch64_feature_flags extra_flags = 0);
> - ~aarch64_simd_switcher ();
> + aarch64_target_switcher (aarch64_feature_flags flags = 0);
> + ~aarch64_target_switcher ();
>
> private:
> aarch64_feature_flags m_old_asm_isa_flags;
> bool m_old_general_regs_only;
> + tree m_old_target_pragma;
> };
>
> /* Represents the ISA requirements of an intrinsic function, or of some
> @@ -1190,6 +1191,7 @@ void aarch64_set_asm_isa_flags (aarch64_feature_flags);
> void aarch64_set_asm_isa_flags (gcc_options *, aarch64_feature_flags);
> bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
> const struct cl_decoded_option *, location_t);
> +aarch64_feature_flags aarch64_get_required_features (aarch64_feature_flags);
> void aarch64_print_hint_for_extensions (const char *);
> void aarch64_print_hint_for_arch (const char *);
> void aarch64_print_hint_for_core (const char *);
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.h
> b/gcc/config/aarch64/aarch64-sve-builtins.h
> index
> 54d213dfe6e0e1cd95e932fc4a04e9cd360f15f5..ea19cfe47bec042e0fb0b4f3c3820b2d37bb222f
> 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.h
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.h
> @@ -824,11 +824,11 @@ public:
>
> /* RAII class for enabling enough SVE features to define the built-in
> types and implement the arm_sve.h pragma. */
> -class sve_switcher : public aarch64_simd_switcher
> +class sve_target_switcher : public aarch64_target_switcher
> {
> public:
> - sve_switcher (aarch64_feature_flags = 0);
> - ~sve_switcher ();
> + sve_target_switcher (aarch64_feature_flags = 0);
> + ~sve_target_switcher ();
>
> private:
> unsigned int m_old_maximum_field_alignment;
> @@ -836,10 +836,10 @@ private:
> };
>
> /* Extends sve_switch enough for defining arm_sme.h. */
> -class sme_switcher : public sve_switcher
> +class sme_target_switcher : public sve_target_switcher
> {
> public:
> - sme_switcher () : sve_switcher (AARCH64_FL_SME) {}
> + sme_target_switcher () : sve_target_switcher (AARCH64_FL_SME) {}
> };
>
> extern const type_suffix_info type_suffixes[NUM_TYPE_SUFFIXES + 1];
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc
> b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index
> 5d2062726d6bab31652bc9fa4bbd597704ef46e5..8e9cb5cea2de1d51a853900f9002550606805052
> 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -1296,8 +1296,8 @@ registered_function_hasher::equal (value_type value,
> const compare_type &key)
> return value->instance == key;
> }
>
> -sve_switcher::sve_switcher (aarch64_feature_flags flags)
> - : aarch64_simd_switcher (AARCH64_FL_F16 | AARCH64_FL_SVE | flags)
> +sve_target_switcher::sve_target_switcher (aarch64_feature_flags flags)
> + : aarch64_target_switcher (AARCH64_FL_SVE | flags)
> {
> /* Changing the ISA flags and have_regs_of_mode should be enough here.
> We shouldn't need to pay the compile-time cost of a full target
> @@ -1312,7 +1312,7 @@ sve_switcher::sve_switcher (aarch64_feature_flags flags)
> have_regs_of_mode[i] = true;
> }
>
> -sve_switcher::~sve_switcher ()
> +sve_target_switcher::~sve_target_switcher ()
> {
> memcpy (have_regs_of_mode, m_old_have_regs_of_mode,
> sizeof (have_regs_of_mode));
> @@ -4726,7 +4726,7 @@ register_builtin_types ()
> void
> init_builtins ()
> {
> - sve_switcher sve;
> + sve_target_switcher switcher;
> register_builtin_types ();
> if (in_lto_p)
> {
> @@ -4842,7 +4842,7 @@ handle_arm_sve_h (bool function_nulls_p)
> return;
> }
>
> - sve_switcher sve;
> + sve_target_switcher switcher;
>
> /* Define the vector and tuple types. */
> for (unsigned int type_i = 0; type_i < NUM_VECTOR_TYPES; ++type_i)
> @@ -4873,6 +4873,8 @@ handle_arm_neon_sve_bridge_h (bool function_nulls_p)
> if (initial_indexes[arm_sme_handle] == 0)
> handle_arm_sme_h (true);
>
> + aarch64_target_switcher switcher;
> +
> /* Define the functions. */
> function_builder builder (arm_neon_sve_handle, function_nulls_p);
> for (unsigned int i = 0; i < ARRAY_SIZE (neon_sve_function_groups); ++i)
> @@ -4900,7 +4902,7 @@ handle_arm_sme_h (bool function_nulls_p)
> return;
> }
>
> - sme_switcher sme;
> + sme_target_switcher switcher;
>
> function_builder builder (arm_sme_handle, function_nulls_p);
> for (unsigned int i = 0; i < ARRAY_SIZE (sme_function_groups); ++i)
