> -----Original Message-----
> From: Richard Sandiford <richard.sandif...@arm.com>
> Sent: 10 July 2020 17:54
> To: gcc-patches@gcc.gnu.org
> Cc: ni...@redhat.com; Richard Earnshaw <richard.earns...@arm.com>;
> Ramana Radhakrishnan <ramana.radhakrish...@arm.com>; Kyrylo
> Tkachov <kyrylo.tkac...@arm.com>
> Subject: [PATCH] arm: Treat GNU and Advanced SIMD vectors as distinct
> [PR92789, PR95726]
>
> This is an arm version of aarch64 patch r11-1741. The approach
> is essentially identical, not much more than s/aarch64/arm/.
>
> To recap, PR95726 is about template look-up for things like:
>
> foo<float vecf __attribute__((vector_size(16)))>
> foo<float32x4_t>
>
> The immediate cause of the problem is that the hash function usually
> returns different hashes for these types, yet the equality function
> thinks they are equal. This then raises the question of how the types
> are supposed to be treated.
>
> The answer we chose for AArch64 was that the GNU vector type should
> be treated as distinct from float32x4_t, but that each type should
> implicitly convert to the other.
>
> This would mean that, as far as the PR is concerned, the hashing
> function is right to (sometimes) treat the types differently and
> the equality function is wrong to treat them as the same.
>
> The most obvious way to enforce the type difference is to use a
> target-specific type attribute. That on its own is enough to fix
> the PR. The difficulty is deciding whether the knock-on effects
> are acceptable.
>
> One obvious effect is that GCC then rejects:
>
> typedef float vecf __attribute__((vector_size(16)));
> vecf x;
> float32x4_t &z = x;
>
> on the basis that the types are no longer reference-compatible.
> For AArch64 we took the approach that this was the correct behaviour.
> It is also consistent with current Clang.
>
> A trickier question is whether:
>
> vecf x;
> float32x4_t y;
> … c ? x : y …
>
> should be valid, and if so, what its type should be [PR92789].
> As explained in the comment in the testcase, GCC and Clang both
> accepted this, but GCC chose the “then” type while Clang chose
> the “else” type. This can lead to different mangling for (probably
> artificial) corner cases, as seen for “sel1” and “sel2” in the
> testcase.
>
> Adding the attribute makes GCC reject the conditional expression
> as ambiguous. For AArch64 we took the approach that this too is
> the correct behaviour, for the reasons described in the testcase.
> However, it does seem to have the potential to break existing code.
>
> Tested on arm-linux-gnueabihf. OK for master? The backport will
> involve an arm version of:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549614.html
>
Ok. I'd like arm gcc to be consistent with aarch64 gcc for sure.
Thanks,
Kyrill
> Richard
>
>
> gcc/
> PR target/92789
> PR target/95726
> * config/arm/arm.c (arm_attribute_table): Add
> "Advanced SIMD type".
> (arm_comp_type_attributes): Check that the "Advanced SIMD type"
> attributes are equal.
> * config/arm/arm-builtins.c: Include stringpool.h and
> attribs.h.
> (arm_mangle_builtin_vector_type): Use the mangling recorded
> in the "Advanced SIMD type" attribute.
> (arm_init_simd_builtin_types): Add an "Advanced SIMD type"
> attribute to each Advanced SIMD type, using the mangled type
> as the attribute's single argument.
>
> gcc/testsuite/
> PR target/92789
> PR target/95726
> * g++.target/arm/pr95726.C: New test.
> ---
> gcc/config/arm/arm-builtins.c | 35 ++++++++++--------
> gcc/config/arm/arm.c | 10 ++++++
> gcc/testsuite/g++.target/arm/pr95726.C | 49
> ++++++++++++++++++++++++++
> 3 files changed, 79 insertions(+), 15 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/arm/pr95726.C
>
> diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
> index f64742e6447..33e8015b140 100644
> --- a/gcc/config/arm/arm-builtins.c
> +++ b/gcc/config/arm/arm-builtins.c
> @@ -43,6 +43,8 @@
> #include "sbitmap.h"
> #include "stringpool.h"
> #include "arm-builtins.h"
> +#include "stringpool.h"
> +#include "attribs.h"
>
> #define SIMD_MAX_BUILTIN_ARGS 7
>
> @@ -1457,18 +1459,12 @@ arm_mangle_builtin_scalar_type (const_tree
> type)
> static const char *
> arm_mangle_builtin_vector_type (const_tree type)
> {
> - int i;
> - int nelts = sizeof (arm_simd_types) / sizeof (arm_simd_types[0]);
> -
> - for (i = 0; i < nelts; i++)
> - if (arm_simd_types[i].mode == TYPE_MODE (type)
> - && TYPE_NAME (type)
> - && TREE_CODE (TYPE_NAME (type)) == TYPE_DECL
> - && DECL_NAME (TYPE_NAME (type))
> - && !strcmp
> - (IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type))),
> - arm_simd_types[i].name))
> - return arm_simd_types[i].mangle;
> + tree attrs = TYPE_ATTRIBUTES (type);
> + if (tree attr = lookup_attribute ("Advanced SIMD type", attrs))
> + {
> + tree mangled_name = TREE_VALUE (TREE_VALUE (attr));
> + return IDENTIFIER_POINTER (mangled_name);
> + }
>
> return NULL;
> }
> @@ -1642,9 +1638,18 @@ arm_init_simd_builtin_types (void)
> if (eltype == NULL)
> continue;
> if (arm_simd_types[i].itype == NULL)
> - arm_simd_types[i].itype =
> - build_distinct_type_copy
> - (build_vector_type (eltype, GET_MODE_NUNITS (mode)));
> + {
> + tree type = build_vector_type (eltype, GET_MODE_NUNITS (mode));
> + type = build_distinct_type_copy (type);
> + SET_TYPE_STRUCTURAL_EQUALITY (type);
> +
> + tree mangled_name = get_identifier (arm_simd_types[i].mangle);
> + tree value = tree_cons (NULL_TREE, mangled_name, NULL_TREE);
> + TYPE_ATTRIBUTES (type)
> + = tree_cons (get_identifier ("Advanced SIMD type"), value,
> + TYPE_ATTRIBUTES (type));
> + arm_simd_types[i].itype = type;
> + }
>
> tdecl = add_builtin_type (arm_simd_types[i].name,
> arm_simd_types[i].itype);
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index ea0ac01e68c..4ed717309ff 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -382,6 +382,7 @@ static const struct attribute_spec
> arm_attribute_table[] =
> arm_handle_cmse_nonsecure_entry, NULL },
> { "cmse_nonsecure_call", 0, 0, true, false, false, true,
> arm_handle_cmse_nonsecure_call, NULL },
> + { "Advanced SIMD type", 1, 1, false, true, false, true, NULL, NULL },
> { NULL, 0, 0, false, false, false, false, NULL, NULL }
> };
>
>
>
> @@ -7539,6 +7540,15 @@ arm_comp_type_attributes (const_tree type1,
> const_tree type2)
> {
> int l1, l2, s1, s2;
>
> + tree attrs1 = lookup_attribute ("Advanced SIMD type",
> + TYPE_ATTRIBUTES (type1));
> + tree attrs2 = lookup_attribute ("Advanced SIMD type",
> + TYPE_ATTRIBUTES (type2));
> + if (bool (attrs1) != bool (attrs2))
> + return 0;
> + if (attrs1 && !attribute_value_equal (attrs1, attrs2))
> + return 0;
> +
> /* Check for mismatch of non-default calling convention. */
> if (TREE_CODE (type1) != FUNCTION_TYPE)
> return 1;
> diff --git a/gcc/testsuite/g++.target/arm/pr95726.C
> b/gcc/testsuite/g++.target/arm/pr95726.C
> new file mode 100644
> index 00000000000..5f7dbf11ddc
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/arm/pr95726.C
> @@ -0,0 +1,49 @@
> +// { dg-require-effective-target arm_neon_ok }
> +// { dg-add-options arm_neon }
> +
> +#include <arm_neon.h>
> +
> +typedef float vecf __attribute__((vector_size(16)));
> +
> +// This assertion must hold: vecf and float32x4_t have distinct identities
> +// and mangle differently, so they are not interchangeable.
> +template<typename T> struct bar;
> +template<> struct bar<vecf> { static const int x = 1; };
> +template<> struct bar<float32x4_t> { static const int x = 2; };
> +static_assert(bar<vecf>::x + bar<float32x4_t>::x == 3, "boo");
> +
> +// GCC 10.1 and earlier accepted this. However, the rule should be
> +// that GNU vectors and Advanced SIMD vectors are distinct types but
> +// that each one implicitly converts to the other. The types are not
> +// reference-compatible.
> +//
> +// The behavior tested below is consistent with Clang.
> +vecf x;
> +float32x4_t y;
> +float32x4_t &z = x; // { dg-error {cannot bind non-const lvalue reference} }
> +
> +// These assignment must be valid even in the strictest mode: vecf must
> +// implicitly convert to float32x4_t and vice versa.
> +void foo() { x = y; y = x; }
> +
> +// Previously GCC accepted this and took the type of "d" from the "then"
> arm.
> +// It therefore mangled the functions as:
> +//
> +// _Z4sel1bRDv4_f
> +// _Z4sel2bR19__simd128_float32_t
> +//
> +// Clang currently also accepts it and takes the type of "d" from the
> +// "else" arm. It therefore mangles the functions as follows, which is
> +// inconsistent with the old GCC behavior:
> +//
> +// _Z4sel1b19__simd128_float32_t
> +// _Z4sel2bDv4_f
> +//
> +// Given that the types have distinct identities and that each one
> +// implicitly converts to the other (see above), the expression ought
> +// to be rejected as invalid. This is consistent (by analogy) with the
> +// standard C++ handling of conditional expressions involving class types,
> +// in cases where the "then" value implicitly converts to the "else" type
> +// and the "else" value implicitly converts to the "then" type.
> +auto sel1(bool c, decltype(c ? x : y) d) { return d; } // { dg-error
> {operands to
> '\?:' have different types} }
> +auto sel2(bool c, decltype(c ? y : x) d) { return d; } // { dg-error
> {operands to
> '\?:' have different types} }
