Andrew Carlotti <andrew.carlo...@arm.com> writes:
> On Wed, Feb 19, 2025 at 12:17:55PM +0000, Richard Sandiford wrote:
>> Andrew Carlotti <andrew.carlo...@arm.com> writes:
>> > /* 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.
Yeah. IMO it's a misfeature that, when compiled with -fpack-struct=2:
#include <arm_neon.h>
int x1 = _Alignof (int32x4_t);
int x2 = _Alignof (int32x4x2_t);
sets x1 to 16 and x2 to 2. But that's been GCC's long-standing behaviour
and is also what Clang does, so I suppose we have to live with it.
> 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).
Yeah, that's right. It happens via target_reinit -> init_regs when
the target is used for the first time. The result is then reused
for subsequent switches to the same target.
> 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?
Yeah, a separate RAII class for the field alignment sounds good to me,
with the have_regs_for_mode stuff moving to aarch64_target_switcher.
Thanks,
Richard
