Andrew Carlotti <andrew.carlo...@arm.com> writes:
> When initialising intrinsics with `#pragma GCC aarch64 "arm_*.h"`, we
> often set an explicit target, but currently leave current_target_pragma
> unchanged. This results in the target pragma being applied to each
> simulated intrinsic on top of our explicit target, which is clearly
> undesirable.
>
> As far as I can tell this doesn't cause any bugs at the moment, because
> none of the behaviour for builtin functions depends upon the function
> specific target. However, the unintended target feature combinations
> led to unwanted behaviour in an under-developement patch.
>
> This patch resolves the issue by extending aarch64_simd_switcher to
> explicitly unset the current_target_pragma, and adapting it for to
> support handle_arm_acle_h as well. I've also renamed the switcher classes
> and instances, because I think the new names a slightly clearer.
>
> The chosen sets of features for arm_sve.h and arm_sme.h are not normally
> valid, because they exclude FCMA and BF16. However, I don't think that
> matters for the usage here. Alternatively, aarch64_target_switcher
> could be modified to enable all the dependent features as well.
>
>
> Bootstrapped and regression tested on aarch64. Ok for master (to enable the
> dependant WIP patch)?
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-builtins.cc
> (aarch64_simd_switcher::aarch64_simd_switcher): Rename to...
> (aarch64_target_switcher::aarch64_target_switcher): ...this,
> remove default simd flags and save current_target_pragma.
> (aarch64_simd_switcher::~aarch64_simd_switcher): Rename to...
> (aarch64_target_switcher::~aarch64_target_switcher): ...this,
> and restore current_target_pragma.
> (handle_arm_acle_h): Use aarch64_target_switcher.
> (handle_arm_neon_h): Rename switcher and pass explicit flags.
> (aarch64_general_init_builtins): Ditto.
> * config/aarch64/aarch64-protos.h
> (class aarch64_simd_switcher): Rename to...
> (class aarch64_target_switcher): ...this, and add pragma member.
> * config/aarch64/aarch64-sve-builtins.cc
> (sve_switcher::sve_switcher): Rename to...
> (sve_target_switcher::sve_target_switcher): ...this.
> (sve_switcher::~sve_switcher): Rename to...
> (sve_target_switcher::~sve_target_switcher): ...this.
> (init_builtins): Rename switcher.
> (handle_arm_sve_h): Ditto.
> (handle_arm_neon_sve_bridge_h): Ditto.
> (handle_arm_sme_h): Ditto.
> * config/aarch64/aarch64-sve-builtins.h
> (class sve_switcher): Rename to...
> (class sve_target_switcher): ...this.
> (class sme_switcher): Rename to...
> (class sme_target_switcher): ...this.
>
>
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc
> b/gcc/config/aarch64/aarch64-builtins.cc
> index
> 128cc365d3d585e01cb69668f285318ee56a36fc..c1cb6cdcc81c6b45c0132250589bba0be42f195d
> 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -1877,23 +1877,25 @@ 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)
> {
> /* 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);
> + aarch64_set_asm_isa_flags (flags);
This feels a bit inconsistent, in that it forces -mgeneral-regs off
but doesn't force AARCH64_FL_FP on. I think it'd be better to keep
this part of aarch64_simd_(target_)switcher (and continue to have
sve_(target_)switcher derive from it) and make aarch64_target_switcher
a new base class that just does the pragma bit.
Thanks,
Richard
> + 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 +1905,7 @@ aarch64_simd_switcher::~aarch64_simd_switcher ()
> void
> handle_arm_neon_h (void)
> {
> - aarch64_simd_switcher simd;
> + aarch64_target_switcher switcher (AARCH64_FL_FP | AARCH64_FL_SIMD);
>
> /* Register the AdvSIMD vector tuple types. */
> for (unsigned int i = 0; i < ARM_NEON_H_TYPES_LAST; i++)
> @@ -2353,6 +2355,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 +2450,7 @@ aarch64_general_init_builtins (void)
> aarch64_init_bf16_types ();
>
> {
> - aarch64_simd_switcher simd;
> + aarch64_target_switcher switcher (AARCH64_FL_FP | 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..cbae258a5bbb3feee80f14add6567f2440cc6927
> 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
> 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..0e3887db74a64326d036f0ef70a35f31dcb971b0
> 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -1296,8 +1296,12 @@ 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_FP
> + | AARCH64_FL_SIMD
> + | AARCH64_FL_F16
> + | 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 +1316,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 +4730,7 @@ register_builtin_types ()
> void
> init_builtins ()
> {
> - sve_switcher sve;
> + sve_target_switcher switcher;
> register_builtin_types ();
> if (in_lto_p)
> {
> @@ -4842,7 +4846,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 +4877,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 +4906,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)
