On Wed, Feb 19, 2025 at 12:17:55PM +0000, Richard Sandiford wrote:
> 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.
I think I want to take a step back and look at what all the switcher effects
are and (to some extent) why they're needed.
1. Setting the value of aarch64_isa_flags - this requires unsetting
MASK_GENERAL_REGS_ONLY (for fp/simd targets), calling aarc64_set_asm_isa_flags,
and (for functions) unsetting current_target_pragma. I'm not sure what breaks
if this isn't done.
2. Setting maximum_field_alignment = 0 (to disable the effect of -fpack-struct)
- I think this is only relevant for ensuring the correct layout of the SVE
tuple types.
3. Enabling have_regs_for_mode[] for the SVE modes - I'm not sure specifically
why this is needed either, but I'm guessing this normally gets enabled as part
of a full target switch (though I couldn't immediately see how).
I think it makes sense to me for 1. and 3. to be based on flags (although I
think we need to base 3. upon either of SVE or SME being specified, to support
decoupling that dependency in the future). I'm not sure the same applies
for 2., however - perhaps it makes more sense for that to be a separate
standalone switcher class that is only used when defining the SVE tuple types?
How does that sound?
> 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)
