From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 6D361395B803 for ; Wed, 14 Apr 2021 15:32:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6D361395B803 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 19170113E; Wed, 14 Apr 2021 08:32:23 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 753CB3F73B; Wed, 14 Apr 2021 08:32:22 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, joseph@codesourcery.com, polacek@redhat.com, richard.sandiford@arm.com Cc: joseph@codesourcery.com, polacek@redhat.com Subject: [PATCH] c: Don't drop vector attributes that affect type identity [PR98852] Date: Wed, 14 Apr 2021 16:32:21 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Apr 2021 15:32:25 -0000 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 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: vector + vector had a GNU vector type rather than an 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 +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 + +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); +}