[RFC] aarch64: Treat GNU and Advanced SIMD vectors as distinct [PR92789, PR95726]

Message ID mpt7dvqcda2.fsf@arm.com
State New
Headers show
Series
  • [RFC] aarch64: Treat GNU and Advanced SIMD vectors as distinct [PR92789, PR95726]
Related show

Commit Message

Richard Sandiford June 29, 2020, 11:38 a.m.
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.

I think the answer is that the GNU vector type should be treated as
distinct from float32x4_t, not least because the two types mangle
differently.  However, 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.
I think that's again the correct behaviour, and 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.  I think that too is the correct behaviour, for the
reasons described in the testcase.  However, it does seem to have
the potential to break existing code.

It looks like aarch64_comp_type_attributes is missing cases for
the SVE attributes, but I'll handle that in a separate patch.

Any thoughts?  I'll apply this after 5pm UTC tomorrow if no asks
me not to. :-)

If I do apply the patch in its current form and there's no fallout,
I'll post a similar one for AArch32.  I'm not sure yet what to do
about backports though -- there does seem to be a too-high risk
of breaking things.

Tested on aarch64-linux-gnu (with and without SVE).

Richard


2020-06-26  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	PR target/92789
	PR target/95726
	* config/aarch64/aarch64.c (aarch64_attribute_table): Add
	"Advanced SIMD type".
	(aarch64_comp_type_attributes): Check that the "Advanced SIMD type"
	attributes are equal.
	* config/aarch64/aarch64-builtins.c: Include stringpool.h and
	attribs.h.
	(aarch64_mangle_builtin_vector_type): Use the mangling recorded
	in the "Advanced SIMD type" attribute.
	(aarch64_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/aarch64/pr95726.C: New test.
---
 gcc/config/aarch64/aarch64-builtins.c      | 34 ++++++++--------
 gcc/config/aarch64/aarch64.c               | 15 ++++++-
 gcc/testsuite/g++.target/aarch64/pr95726.C | 46 ++++++++++++++++++++++
 3 files changed, 77 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/aarch64/pr95726.C

Comments

Peter Bergner via Gcc-patches June 29, 2020, 12:12 p.m. | #1
On Mon, Jun 29, 2020 at 12:38:45PM +0100, Richard Sandiford wrote:
> It looks like aarch64_comp_type_attributes is missing cases for

> the SVE attributes, but I'll handle that in a separate patch.

> 

> Any thoughts?  I'll apply this after 5pm UTC tomorrow if no asks

> me not to. :-)

> 

> If I do apply the patch in its current form and there's no fallout,

> I'll post a similar one for AArch32.  I'm not sure yet what to do

> about backports though -- there does seem to be a too-high risk

> of breaking things.


I believe Jason said to do it only if comparing_specializations is set,
the problem is that comparing_specializations is defined in the C++ FE
and the hooks can be linked without the C++ FE being linked in.
Perhaps for backports only move the int comparing_specializations;
definition out from the C++ FE to some generic file (tree.c) with
a comment that it is an ugly hack?  Non-C++ FEs would just keep the
variable 0 all the time and thus no change except when the C++ FE
compares template arguments?

Thanks for working on this.

> 2020-06-26  Richard Sandiford  <richard.sandiford@arm.com>

> 

> gcc/

> 	PR target/92789

> 	PR target/95726

> 	* config/aarch64/aarch64.c (aarch64_attribute_table): Add

> 	"Advanced SIMD type".

> 	(aarch64_comp_type_attributes): Check that the "Advanced SIMD type"

> 	attributes are equal.

> 	* config/aarch64/aarch64-builtins.c: Include stringpool.h and

> 	attribs.h.

> 	(aarch64_mangle_builtin_vector_type): Use the mangling recorded

> 	in the "Advanced SIMD type" attribute.

> 	(aarch64_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/aarch64/pr95726.C: New test.


	Jakub

Patch

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 95213cd70c8..e87a4559c36 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
@@ -639,18 +641,12 @@  aarch64_mangle_builtin_scalar_type (const_tree type)
 static const char *
 aarch64_mangle_builtin_vector_type (const_tree type)
 {
-  int i;
-  int nelts = sizeof (aarch64_simd_types) / sizeof (aarch64_simd_types[0]);
-
-  for (i = 0; i < nelts; i++)
-    if (aarch64_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))),
-	      aarch64_simd_types[i].name))
-      return aarch64_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;
 }
@@ -802,10 +798,16 @@  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);
+
+	  tree mangled_name = get_identifier (aarch64_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));
+	  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 f3551a73d87..57988f9330b 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", 1, 1, 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 }
@@ -22721,8 +22722,18 @@  aarch64_simd_clone_usable (struct cgraph_node *node)
 static int
 aarch64_comp_type_attributes (const_tree type1, const_tree type2)
 {
-  if (lookup_attribute ("aarch64_vector_pcs", TYPE_ATTRIBUTES (type1))
-      != lookup_attribute ("aarch64_vector_pcs", TYPE_ATTRIBUTES (type2)))
+  auto check_attr = [&](const char *name) {
+    tree attr1 = lookup_attribute (name, TYPE_ATTRIBUTES (type1));
+    tree attr2 = lookup_attribute (name, TYPE_ATTRIBUTES (type2));
+    if (!attr1 && !attr2)
+      return true;
+
+    return attr1 && attr2 && attribute_value_equal (attr1, attr2);
+  };
+
+  if (!check_attr ("aarch64_vector_pcs"))
+    return 0;
+  if (!check_attr ("Advanced SIMD type"))
     return 0;
   return 1;
 }
diff --git a/gcc/testsuite/g++.target/aarch64/pr95726.C b/gcc/testsuite/g++.target/aarch64/pr95726.C
new file mode 100644
index 00000000000..ddd69b8b0da
--- /dev/null
+++ b/gcc/testsuite/g++.target/aarch64/pr95726.C
@@ -0,0 +1,46 @@ 
+#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
+//   _Z4sel2bR13__Float32x4_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:
+//
+//   _Z4sel1b13__Float32x4_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} }