* [PATCH] c: Don't drop vector attributes that affect type identity [PR98852]
@ 2021-04-14 15:32 Richard Sandiford
2021-04-14 20:22 ` Joseph Myers
2021-04-14 20:24 ` Jeff Law
0 siblings, 2 replies; 3+ messages in thread
From: Richard Sandiford @ 2021-04-14 15:32 UTC (permalink / raw)
To: gcc-patches; +Cc: joseph, polacek
<arm_neon.h> types are distinct from GNU vector types in at least
their mangling. However, there used to be nothing explicit in the
VECTOR_TYPE itself to indicate the difference: we simply treated them
as distinct TYPE_MAIN_VARIANTs. This caused problems like the ones
reported in PR95726.
The fix for that PR was to add type attributes to the <arm_neon.h>
types, in order to maintain the distinction between them and GNU
vectors. However, this in turn caused PR98852, where c_common_type
would unconditionally drop the attributes on the source types.
This meant that:
<arm_neon.h> vector + <arm_neon.h> vector
had a GNU vector type rather than an <arm_neon.h> vector type.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96377#c2 for
Jakub's analysis of the history of this c_common_type code.
TBH I'm not sure which case the build_type_attribute_variant
code is handling, but I think we should at least avoid dropping
attributes that affect type identity.
I've tried to audit the C and target-specific attributes to look
for other types that might be affected by this, but I couldn't
see any. We are only dealing with:
gcc_assert (code1 == VECTOR_TYPE || code1 == COMPLEX_TYPE
|| code1 == FIXED_POINT_TYPE || code1 == REAL_TYPE
|| code1 == INTEGER_TYPE);
which excludes most affects_type_identity attributes. The closest
was s390_vector_bool, but the handler for that attribute changes
the type node and drops the attribute itself (*no_add_attrs = true).
I put the main list handling into a separate function
(remove_attributes_matching) because a later patch will need it
for something else.
Tested on aarch64-linux-gnu, arm-linux-gnueabihf, armeb-eabi and
x86_64-linux-gnu. OK for trunk? The bug also occurs on GCC 10 branch,
but we'll need a slightly different fix there.
Thanks,
Richard
gcc/
PR c/98852
* attribs.h (affects_type_identity_attributes): Declare.
* attribs.c (remove_attributes_matching): New function.
(affects_type_identity_attributes): Likewise.
gcc/c/
PR c/98852
* c-typeck.c (c_common_type): Do not drop attributes that
affect type identity.
gcc/testsuite/
PR c/98852
* gcc.target/aarch64/advsimd-intrinsics/pr98852.c: New test.
---
gcc/attribs.c | 54 ++++++++
gcc/attribs.h | 2 +
gcc/c/c-typeck.c | 10 +-
.../aarch64/advsimd-intrinsics/pr98852.c | 129 ++++++++++++++++++
4 files changed, 193 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/pr98852.c
diff --git a/gcc/attribs.c b/gcc/attribs.c
index 16c6b12d477..2fb29541f3f 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -1366,6 +1366,60 @@ comp_type_attributes (const_tree type1, const_tree type2)
return targetm.comp_type_attributes (type1, type2);
}
+/* PREDICATE acts as a function of type:
+
+ (const_tree attr, const attribute_spec *as) -> bool
+
+ where ATTR is an attribute and AS is its possibly-null specification.
+ Return a list of every attribute in attribute list ATTRS for which
+ PREDICATE is true. Return ATTRS itself if PREDICATE returns true
+ for every attribute. */
+
+template<typename Predicate>
+tree
+remove_attributes_matching (tree attrs, Predicate predicate)
+{
+ tree new_attrs = NULL_TREE;
+ tree *ptr = &new_attrs;
+ const_tree start = attrs;
+ for (const_tree attr = attrs; attr; attr = TREE_CHAIN (attr))
+ {
+ tree name = get_attribute_name (attr);
+ const attribute_spec *as = lookup_attribute_spec (name);
+ const_tree end;
+ if (!predicate (attr, as))
+ end = attr;
+ else if (start == attrs)
+ continue;
+ else
+ end = TREE_CHAIN (attr);
+
+ for (; start != end; start = TREE_CHAIN (start))
+ {
+ *ptr = tree_cons (TREE_PURPOSE (start),
+ TREE_VALUE (start), NULL_TREE);
+ TREE_CHAIN (*ptr) = NULL_TREE;
+ ptr = &TREE_CHAIN (*ptr);
+ }
+ start = TREE_CHAIN (attr);
+ }
+ gcc_assert (!start || start == attrs);
+ return start ? attrs : new_attrs;
+}
+
+/* If VALUE is true, return the subset of ATTRS that affect type identity,
+ otherwise return the subset of ATTRS that don't affect type identity. */
+
+tree
+affects_type_identity_attributes (tree attrs, bool value)
+{
+ auto predicate = [value](const_tree, const attribute_spec *as) -> bool
+ {
+ return bool (as && as->affects_type_identity) == value;
+ };
+ return remove_attributes_matching (attrs, predicate);
+}
+
/* Return a type like TTYPE except that its TYPE_ATTRIBUTE
is ATTRIBUTE.
diff --git a/gcc/attribs.h b/gcc/attribs.h
index 898e73db3e4..eadb1d0fac9 100644
--- a/gcc/attribs.h
+++ b/gcc/attribs.h
@@ -65,6 +65,8 @@ extern bool attribute_value_equal (const_tree, const_tree);
warning to be generated). */
extern int comp_type_attributes (const_tree, const_tree);
+extern tree affects_type_identity_attributes (tree, bool = true);
+
/* Default versions of target-overridable functions. */
extern tree merge_decl_attributes (tree, tree);
extern tree merge_type_attributes (tree, tree);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 21eab00d4b3..51a62c800f7 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -740,10 +740,16 @@ c_common_type (tree t1, tree t2)
t2 = TYPE_MAIN_VARIANT (t2);
if (TYPE_ATTRIBUTES (t1) != NULL_TREE)
- t1 = build_type_attribute_variant (t1, NULL_TREE);
+ {
+ tree attrs = affects_type_identity_attributes (TYPE_ATTRIBUTES (t1));
+ t1 = build_type_attribute_variant (t1, attrs);
+ }
if (TYPE_ATTRIBUTES (t2) != NULL_TREE)
- t2 = build_type_attribute_variant (t2, NULL_TREE);
+ {
+ tree attrs = affects_type_identity_attributes (TYPE_ATTRIBUTES (t2));
+ t2 = build_type_attribute_variant (t2, attrs);
+ }
/* Save time if the two types are the same. */
diff --git a/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/pr98852.c b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/pr98852.c
new file mode 100644
index 00000000000..31e51b0d893
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/pr98852.c
@@ -0,0 +1,129 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-std=c99" } */
+
+#include <arm_neon.h>
+
+typedef __typeof(((int32x4_t *) 0)[0][0]) int32_elt;
+typedef __typeof(((uint32x4_t *) 0)[0][0]) uint32_elt;
+
+typedef int32_elt gnu_int32x4_t __attribute__((vector_size(16)));
+typedef uint32_elt gnu_uint32x4_t __attribute__((vector_size(16)));
+
+#define X_gnu_int32x4_t 1
+#define X_gnu_uint32x4_t 2
+#define X_int32x4_t 3
+#define X_uint32x4_t 4
+
+#define CHECK(T) T: X_##T
+
+#define CHECK_TYPE(EXPR, TYPE) \
+ do { \
+ int x[_Generic (EXPR, \
+ CHECK (gnu_int32x4_t), \
+ CHECK (gnu_uint32x4_t), \
+ CHECK (int32x4_t), \
+ CHECK (uint32x4_t), \
+ default : 0) == X_##TYPE ? 1 : -1]; \
+ } while (0)
+
+void
+f (gnu_int32x4_t sg, gnu_uint32x4_t ug, int32x4_t sn, uint32x4_t un, int c)
+{
+ CHECK_TYPE (sg, gnu_int32x4_t);
+ CHECK_TYPE (ug, gnu_uint32x4_t);
+ CHECK_TYPE (sn, int32x4_t);
+ CHECK_TYPE (un, uint32x4_t);
+
+ CHECK_TYPE (sg + 1, gnu_int32x4_t);
+ CHECK_TYPE (ug + 1, gnu_uint32x4_t);
+ CHECK_TYPE (sn + 1, int32x4_t);
+ CHECK_TYPE (un + 1, uint32x4_t);
+
+ CHECK_TYPE (1 + sg, gnu_int32x4_t);
+ CHECK_TYPE (1 + ug, gnu_uint32x4_t);
+ CHECK_TYPE (1 + sn, int32x4_t);
+ CHECK_TYPE (1 + un, uint32x4_t);
+
+ CHECK_TYPE (sg + sg, gnu_int32x4_t);
+ CHECK_TYPE (ug + ug, gnu_uint32x4_t);
+ CHECK_TYPE (sn + sn, int32x4_t);
+ CHECK_TYPE (un + un, uint32x4_t);
+
+ /* Traditional behavior for mixed signs is to pick the signedness of the
+ first operand. We don't have any Arm-specific reason for preferring that
+ behavior, but including the tests helps to demonstrate the points in the
+ comments below. */
+ CHECK_TYPE (sg + ug, gnu_int32x4_t);
+ CHECK_TYPE (ug + sg, gnu_uint32x4_t);
+ CHECK_TYPE (sn + un, int32x4_t);
+ CHECK_TYPE (un + sn, uint32x4_t);
+
+ /* Nothing specifies the type of mixed GNU and arm_neon.h operations, but:
+
+ - it would be surprising if sg + un had a different signedness from
+ sg + ug
+
+ - it would also be mildly surprising if sg + un had a different type from
+ both of its operands
+
+ So in cases where the operands differ in both signedness and ABI, it seems
+ more consistent to ignore the ABI difference and apply the usual rules for
+ differences in sign. */
+ CHECK_TYPE (sg + un, gnu_int32x4_t);
+ CHECK_TYPE (ug + sn, gnu_uint32x4_t);
+ CHECK_TYPE (sn + ug, int32x4_t);
+ CHECK_TYPE (un + sg, uint32x4_t);
+
+ /* And if the first vector wins when operands differ in both signedness
+ and ABI, it seems more consistent to do the same if the operands differ
+ only in ABI. */
+ CHECK_TYPE (sg + sn, gnu_int32x4_t);
+ CHECK_TYPE (ug + un, gnu_uint32x4_t);
+ CHECK_TYPE (sn + sg, int32x4_t);
+ CHECK_TYPE (un + ug, uint32x4_t);
+
+ CHECK_TYPE (c ? sg + sg : sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug + ug : ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn + sn : sn, int32x4_t);
+ CHECK_TYPE (c ? un + un : un, uint32x4_t);
+
+ CHECK_TYPE (c ? sg + 1 : sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug + 1 : ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn + 1 : sn, int32x4_t);
+ CHECK_TYPE (c ? un + 1 : un, uint32x4_t);
+
+ CHECK_TYPE (c ? 1 + sg : sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? 1 + ug : ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? 1 + sn : sn, int32x4_t);
+ CHECK_TYPE (c ? 1 + un : un, uint32x4_t);
+
+ CHECK_TYPE (c ? sg : sg + sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug : ug + ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn : sn + sn, int32x4_t);
+ CHECK_TYPE (c ? un : un + un, uint32x4_t);
+
+ CHECK_TYPE (c ? sg : sg + 1, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug : ug + 1, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn : sn + 1, int32x4_t);
+ CHECK_TYPE (c ? un : un + 1, uint32x4_t);
+
+ CHECK_TYPE (c ? sg : 1 + sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug : 1 + ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn : 1 + sn, int32x4_t);
+ CHECK_TYPE (c ? un : 1 + un, uint32x4_t);
+
+ CHECK_TYPE (c ? sg + sg : sg + sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug + ug : ug + ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn + sn : sn + sn, int32x4_t);
+ CHECK_TYPE (c ? un + un : un + un, uint32x4_t);
+
+ CHECK_TYPE (c ? sg + sg : sg + 1, gnu_int32x4_t);
+ CHECK_TYPE (c ? ug + ug : ug + 1, gnu_uint32x4_t);
+ CHECK_TYPE (c ? sn + sn : sn + 1, int32x4_t);
+ CHECK_TYPE (c ? un + un : un + 1, uint32x4_t);
+
+ CHECK_TYPE (c ? 1 + sg : sg + sg, gnu_int32x4_t);
+ CHECK_TYPE (c ? 1 + ug : ug + ug, gnu_uint32x4_t);
+ CHECK_TYPE (c ? 1 + sn : sn + sn, int32x4_t);
+ CHECK_TYPE (c ? 1 + un : un + un, uint32x4_t);
+}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] c: Don't drop vector attributes that affect type identity [PR98852]
2021-04-14 15:32 [PATCH] c: Don't drop vector attributes that affect type identity [PR98852] Richard Sandiford
@ 2021-04-14 20:22 ` Joseph Myers
2021-04-14 20:24 ` Jeff Law
1 sibling, 0 replies; 3+ messages in thread
From: Joseph Myers @ 2021-04-14 20:22 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches, polacek
On Wed, 14 Apr 2021, Richard Sandiford via Gcc-patches wrote:
> gcc/
> PR c/98852
> * attribs.h (affects_type_identity_attributes): Declare.
> * attribs.c (remove_attributes_matching): New function.
> (affects_type_identity_attributes): Likewise.
>
> gcc/c/
> PR c/98852
> * c-typeck.c (c_common_type): Do not drop attributes that
> affect type identity.
>
> gcc/testsuite/
> PR c/98852
> * gcc.target/aarch64/advsimd-intrinsics/pr98852.c: New test.
OK.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] c: Don't drop vector attributes that affect type identity [PR98852]
2021-04-14 15:32 [PATCH] c: Don't drop vector attributes that affect type identity [PR98852] Richard Sandiford
2021-04-14 20:22 ` Joseph Myers
@ 2021-04-14 20:24 ` Jeff Law
1 sibling, 0 replies; 3+ messages in thread
From: Jeff Law @ 2021-04-14 20:24 UTC (permalink / raw)
To: gcc-patches, joseph, polacek, richard.sandiford
On 4/14/2021 9:32 AM, Richard Sandiford via Gcc-patches wrote:
> <arm_neon.h> types are distinct from GNU vector types in at least
> their mangling. However, there used to be nothing explicit in the
> VECTOR_TYPE itself to indicate the difference: we simply treated them
> as distinct TYPE_MAIN_VARIANTs. This caused problems like the ones
> reported in PR95726.
>
> The fix for that PR was to add type attributes to the <arm_neon.h>
> types, in order to maintain the distinction between them and GNU
> vectors. However, this in turn caused PR98852, where c_common_type
> would unconditionally drop the attributes on the source types.
> This meant that:
>
> <arm_neon.h> vector + <arm_neon.h> vector
>
> had a GNU vector type rather than an <arm_neon.h> vector type.
>
> See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96377#c2 for
> Jakub's analysis of the history of this c_common_type code.
> TBH I'm not sure which case the build_type_attribute_variant
> code is handling, but I think we should at least avoid dropping
> attributes that affect type identity.
>
> I've tried to audit the C and target-specific attributes to look
> for other types that might be affected by this, but I couldn't
> see any. We are only dealing with:
>
> gcc_assert (code1 == VECTOR_TYPE || code1 == COMPLEX_TYPE
> || code1 == FIXED_POINT_TYPE || code1 == REAL_TYPE
> || code1 == INTEGER_TYPE);
>
> which excludes most affects_type_identity attributes. The closest
> was s390_vector_bool, but the handler for that attribute changes
> the type node and drops the attribute itself (*no_add_attrs = true).
>
> I put the main list handling into a separate function
> (remove_attributes_matching) because a later patch will need it
> for something else.
>
> Tested on aarch64-linux-gnu, arm-linux-gnueabihf, armeb-eabi and
> x86_64-linux-gnu. OK for trunk? The bug also occurs on GCC 10 branch,
> but we'll need a slightly different fix there.
>
> Thanks,
> Richard
>
>
> gcc/
> PR c/98852
> * attribs.h (affects_type_identity_attributes): Declare.
> * attribs.c (remove_attributes_matching): New function.
> (affects_type_identity_attributes): Likewise.
>
> gcc/c/
> PR c/98852
> * c-typeck.c (c_common_type): Do not drop attributes that
> affect type identity.
>
> gcc/testsuite/
> PR c/98852
> * gcc.target/aarch64/advsimd-intrinsics/pr98852.c: New test.
OK
Jeff
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-14 20:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 15:32 [PATCH] c: Don't drop vector attributes that affect type identity [PR98852] Richard Sandiford
2021-04-14 20:22 ` Joseph Myers
2021-04-14 20:24 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).