Jakub Jelinek <ja...@redhat.com> writes:
> On Wed, Jul 08, 2020 at 03:10:14PM +0100, Richard Sandiford wrote:
>> gcc/
>> PR target/95726
>> * config/aarch64/aarch64.c (aarch64_attribute_table): Add
>> "Advanced SIMD type".
>> * config/aarch64/aarch64-builtins.c: Include stringpool.h and
>> attribs.h.
>> (aarch64_init_simd_builtin_types): Add an "Advanced SIMD type"
>> attribute to each Advanced SIMD type.
>>
>> gcc/cp/
>> PR target/95726
>> * typeck.c (structural_comptypes): When comparing template
>> specializations, differentiate between vectors that have and
>> do not have an "Advanced SIMD type" attribute.
>>
>> gcc/testsuite/
>> PR target/95726
>> * g++.target/aarch64/pr95726.C: New test.
>> --- a/gcc/cp/typeck.c
>> +++ b/gcc/cp/typeck.c
>> @@ -1429,6 +1429,15 @@ structural_comptypes (tree t1, tree t2, int strict)
>> || maybe_ne (TYPE_VECTOR_SUBPARTS (t1), TYPE_VECTOR_SUBPARTS (t2))
>> || !same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
>> return false;
>
> I'd at least add an explaining comment that it is a hack for GCC 8-10 only,
> for aarch64 and arm targets, why, reference to the PR and that it is solved
> differently for GCC 11+.
OK, done below. This version also includes arm support.
>> + if (comparing_specializations)
>> + {
>> + bool asimd1 = lookup_attribute ("Advanced SIMD type",
>> + TYPE_ATTRIBUTES (t1));
>> + bool asimd2 = lookup_attribute ("Advanced SIMD type",
>> + TYPE_ATTRIBUTES (t2));
>> + if (asimd1 != asimd2)
>> + return false;
>> + }
>
> Otherwise LGTM for release branches if it is acceptable that way to Jason.
Thanks. Jason, does it look OK from your point of view?
Richard
This is a release branch version of
r11-1741-g:31427b974ed7b7dd54e28fec595e731bf6eea8ba and
r11-2022-g:efe99cca78215e339ba79f0a900a896b4c0a3d36.
The trunk versions of the patch made GNU and Advanced SIMD vectors
distinct (but inter-convertible) in all cases. However, the
traditional behaviour is that the types are distinct in template
arguments but not otherwise.
Following a suggestion from Jason, this patch puts the check
for different vector types under comparing_specializations.
In order to keep the backport as simple as possible, the patch
hard-codes the name of the attribute in the frontend rather than
adding a new branch-only target hook.
I didn't find a test that tripped the assert on the branch,
even with the --param in the PR, so instead I tested this by
forcing the hash function to only hash the tree code. That made
the static assertion in the test fail without the patch but pass
with it.
This means that the tests pass for unmodified sources even
without the patch (unless you're very unlucky).
gcc/
PR target/95726
* config/aarch64/aarch64.c (aarch64_attribute_table): Add
"Advanced SIMD type".
* config/aarch64/aarch64-builtins.c: Include stringpool.h and
attribs.h.
(aarch64_init_simd_builtin_types): Add an "Advanced SIMD type"
attribute to each Advanced SIMD type.
* config/arm/arm.c (arm_attribute_table): Add "Advanced SIMD type".
* config/arm/arm-builtins.c: Include stringpool.h and attribs.h.
(arm_init_simd_builtin_types): Add an "Advanced SIMD type"
attribute to each Advanced SIMD type.
gcc/cp/
PR target/95726
* typeck.c (structural_comptypes): When comparing template
specializations, differentiate between vectors that have and
do not have an "Advanced SIMD type" attribute.
gcc/testsuite/
PR target/95726
* g++.target/aarch64/pr95726.C: New test.
* g++.target/arm/pr95726.C: Likewise.
---
gcc/config/aarch64/aarch64-builtins.c | 14 +++++---
gcc/config/aarch64/aarch64.c | 1 +
gcc/config/arm/arm-builtins.c | 15 ++++++--
gcc/config/arm/arm.c | 1 +
gcc/cp/typeck.c | 42 ++++++++++++++++++++++
gcc/testsuite/g++.target/aarch64/pr95726.C | 28 +++++++++++++++
gcc/testsuite/g++.target/arm/pr95726.C | 31 ++++++++++++++++
7 files changed, 125 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/g++.target/aarch64/pr95726.C
create mode 100644 gcc/testsuite/g++.target/arm/pr95726.C
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 5f8f8290f0f..6ebf6319efd 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1429,6 +1429,48 @@ structural_comptypes (tree t1, tree t2, int strict)
|| maybe_ne (TYPE_VECTOR_SUBPARTS (t1), TYPE_VECTOR_SUBPARTS (t2))
|| !same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
return false;
+
+ /* This hack is specific to release branches and fixes PR95726 for
+ arm and aarch64 targets. The idea is to treat GNU and Advanced
+ SIMD types as distinct types for template specialization, but to
+ treat them as the same type for other purposes. For example:
+
+ typedef float vecf __attribute__((vector_size(16)));
+ 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");
+
+ is supposed to be valid, and so "vecf" and "float32x4_t" should
+ be different for at least template specialization. However,
+ GCC 10 and earlier also allowed things like:
+
+ vecf x;
+ float32x4_t &z = x;
+
+ and:
+
+ vecf x;
+ float32x4_t y;
+ ... c ? x : y ...
+
+ both of which require "vecf" and "float32x4_t" to be the same.
+
+ This was fixed in GCC 11+ by making the types distinct in all
+ contexts, making the reference binding and ?: expression above
+ invalid. However, it would be inappropriate to change the
+ semantics like that in release branches, so we do this instead.
+ The space in the attribute name prevents any clash with user-
+ specified attributes. */
+ if (comparing_specializations)
+ {
+ bool asimd1 = lookup_attribute ("Advanced SIMD type",
+ TYPE_ATTRIBUTES (t1));
+ bool asimd2 = lookup_attribute ("Advanced SIMD type",
+ TYPE_ATTRIBUTES (t2));
+ if (asimd1 != asimd2)
+ return false;
+ }
break;
case TYPE_PACK_EXPANSION:
diff --git a/gcc/config/aarch64/aarch64-builtins.c
b/gcc/config/aarch64/aarch64-builtins.c
index 95213cd70c8..8407a34b594 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -43,6 +43,8 @@
#include "gimple-iterator.h"
#include "case-cfn-macros.h"
#include "emit-rtl.h"
+#include "stringpool.h"
+#include "attribs.h"
#define v8qi_UP E_V8QImode
#define v4hi_UP E_V4HImode
@@ -802,10 +804,14 @@ aarch64_init_simd_builtin_types (void)
if (aarch64_simd_types[i].itype == NULL)
{
- aarch64_simd_types[i].itype
- = build_distinct_type_copy
- (build_vector_type (eltype, GET_MODE_NUNITS (mode)));
- SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
+ tree type = build_vector_type (eltype, GET_MODE_NUNITS (mode));
+ type = build_distinct_type_copy (type);
+ SET_TYPE_STRUCTURAL_EQUALITY (type);
+
+ TYPE_ATTRIBUTES (type)
+ = tree_cons (get_identifier ("Advanced SIMD type"),
+ NULL_TREE, TYPE_ATTRIBUTES (type));
+ aarch64_simd_types[i].itype = type;
}
tdecl = add_builtin_type (aarch64_simd_types[i].name,
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index cda494c4bbf..cd67dcfba37 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1429,6 +1429,7 @@ static const struct attribute_spec
aarch64_attribute_table[] =
{ "arm_sve_vector_bits", 1, 1, false, true, false, true,
aarch64_sve::handle_arm_sve_vector_bits_attribute,
NULL },
+ { "Advanced SIMD type", 0, 0, false, true, false, true, NULL, NULL },
{ "SVE type", 3, 3, false, true, false, true, NULL, NULL
},
{ "SVE sizeless type", 0, 0, false, true, false, true, NULL, NULL },
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
index f64742e6447..658dacfb5bf 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
@@ -1642,9 +1644,16 @@ 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);
+
+ TYPE_ATTRIBUTES (type)
+ = tree_cons (get_identifier ("Advanced SIMD type"),
+ NULL_TREE, 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 3f12dbfbfc9..a8825ee3ee4 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", 0, 0, false, true, false, true, NULL, NULL },
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
diff --git a/gcc/testsuite/g++.target/aarch64/pr95726.C
b/gcc/testsuite/g++.target/aarch64/pr95726.C
new file mode 100644
index 00000000000..3327b335ff5
--- /dev/null
+++ b/gcc/testsuite/g++.target/aarch64/pr95726.C
@@ -0,0 +1,28 @@
+#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 and earlier should continue to accept this, but the behavior
+// changed in GCC 11.
+vecf x;
+float32x4_t y;
+float32x4_t &z = x;
+
+// 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; }
+
+// GCC 10 and earlier should continue to accept this, but the behavior
+// changed in GCC 11.
+auto sel1(bool c, decltype(c ? x : y) d) { return d; }
+auto sel2(bool c, decltype(c ? y : x) d) { return d; }
+
+/* { dg-final { scan-assembler {_Z4sel1bRDv4_f} } } */
+/* { dg-final { scan-assembler {_Z4sel2bR13__Float32x4_t} } } */
diff --git a/gcc/testsuite/g++.target/arm/pr95726.C
b/gcc/testsuite/g++.target/arm/pr95726.C
new file mode 100644
index 00000000000..1db15e1030b
--- /dev/null
+++ b/gcc/testsuite/g++.target/arm/pr95726.C
@@ -0,0 +1,31 @@
+// { 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 and earlier should continue to accept this, but the behavior
+// changed in GCC 11.
+vecf x;
+float32x4_t y;
+float32x4_t &z = x;
+
+// 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; }
+
+// GCC 10 and earlier should continue to accept this, but the behavior
+// changed in GCC 11.
+auto sel1(bool c, decltype(c ? x : y) d) { return d; }
+auto sel2(bool c, decltype(c ? y : x) d) { return d; }
+
+/* { dg-final { scan-assembler {_Z4sel1bRDv4_f} } } */
+/* { dg-final { scan-assembler {_Z4sel2bR19__simd128_float32_t} } } */
