On Tue, Jul 14, 2020 at 1:21 AM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> 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).
Can you please apply this patch so I can roll GCC 10.2 RC1?
In the unlikely event Jason requests changes we can integrate
those early next week.
Thanks,
Richard.
> 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} } } */
