On Fri, Feb 21, 2025 at 11:28:14AM +0000, Richard Sandiford wrote:
> 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.
I'm glad that we've iterated to something we're both happy with - I'll post a
v3 for review later.
>
> Thanks,
> Richard
