From efe99cca78215e339ba79f0a900a896b4c0a3d36 Mon Sep 17 00:00:00 2001 From: Richard Sandiford Date: Fri, 10 Jul 2020 19:06:45 +0100 Subject: [PATCH] arm: Treat GNU and Advanced SIMD vectors as distinct [PR92789, PR95726] MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 foo 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. 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 + +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 struct bar; +template<> struct bar { static const int x = 1; }; +template<> struct bar { static const int x = 2; }; +static_assert(bar::x + bar::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} } -- 2.30.2