Message ID | Y/21AVDEBex5vqmc@tucnak |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id AA5043858C2B for <patchwork@sourceware.org>; Tue, 28 Feb 2023 08:02:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org AA5043858C2B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1677571379; bh=V4uOr1Iu3OmYpBBOVy01pj7jmo/djcOhLAzlxzOR5c4=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=AY/QeAT0JZTsIWe0WoWon77DL6PC7GtTP3HGbm0OCyC/b59tPfJDRF3ankMsussmB nVA12TWQkXXBe+9t/hABtgCz0BT2bOoikhYIRoyGYgimlk8Mgjx+SHCYPHBaGurSQ8 ITSi0ZRLX4dCZzAa+FnT8lfDjtBO51dHiCQtTJPw= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 819E43858D39 for <gcc-patches@gcc.gnu.org>; Tue, 28 Feb 2023 08:02:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 819E43858D39 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-222-FUsSIYm4OHaFJlUdz9Q3zw-1; Tue, 28 Feb 2023 03:02:13 -0500 X-MC-Unique: FUsSIYm4OHaFJlUdz9Q3zw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C685F85CBE5; Tue, 28 Feb 2023 08:02:12 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.45.224.101]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8E315422A9; Tue, 28 Feb 2023 08:02:12 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 31S82AjH1108195 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 28 Feb 2023 09:02:10 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 31S829cU1108194; Tue, 28 Feb 2023 09:02:09 +0100 Date: Tue, 28 Feb 2023 09:02:09 +0100 To: Richard Biener <rguenther@suse.de> Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] lto: Fix up lto_fixup_prevailing_type [PR108910] Message-ID: <Y/21AVDEBex5vqmc@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
lto: Fix up lto_fixup_prevailing_type [PR108910]
|
|
Commit Message
Jakub Jelinek
Feb. 28, 2023, 8:02 a.m. UTC
Hi! Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained inside of build_{pointer,reference}_type_for_mode and those routines ensure that the pointer/reference type added to the chain is really unqualified (including address space), without extra user alignment and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL pair (unless something would modify the types in place, but that would be wrong). Now, LTO adds stuff to these chains in lto_fixup_prevailing_type but doesn't guarantee that. The testcase in the PR (which I'm not including for testsuite because when (I hope) the aarch64 backend bug will be fixed, the testcase would work either way) shows a case where user has TYPE_USER_ALIGN type with very high alignment, as there aren't enough pointers to float in the code left that one becomes the prevailing one, lto_fixup_prevailing_type puts it into the TYPE_POINTER_TO chain of float and later on during expansion of __builtin_cexpif expander uses build_pointer_type (float_type_node) to emit a sincosf call and instead of getting a normal pointer type gets this non-standard one. The following patch fixes that by not adding into those chains qualified or user aligned types and by making sure that some type for the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL combination (e.g. from lto1 initialization) isn't there already before adding a new one. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2023-02-28 Jakub Jelinek <jakub@redhat.com> PR target/108910 * lto-common.cc (lto_fixup_prevailing_type): Don't add t to TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has non-zero TYPE_QUALS, or TYPE_USER_ALIGN or some other type with the same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already present. Jakub
Comments
On Tue, 28 Feb 2023, Jakub Jelinek wrote: > Hi! > > Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained > inside of build_{pointer,reference}_type_for_mode and those routines > ensure that the pointer/reference type added to the chain is really > unqualified (including address space), without extra user alignment > and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL > pair (unless something would modify the types in place, but that would > be wrong). Is that so? I can't find any code verifying that (verify_type?). Of course build_{pointer,reference}_type_for_mode will always build unqualified pointers, but then the LTO code does /* The following reconstructs the pointer chains of the new pointed-to type if we are a main variant. We do not stream those so they are broken before fixup. */ if (TREE_CODE (t) == POINTER_TYPE && TYPE_MAIN_VARIANT (t) == t) { TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); TYPE_POINTER_TO (TREE_TYPE (t)) = t; } else if (TREE_CODE (t) == REFERENCE_TYPE && TYPE_MAIN_VARIANT (t) == t) { TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; } which was supposed to ensure only putting unqualified pointers (not pointed to types!) to the chain. So to me the question is rather why a type with TYPE_USER_ALIGN is a the main variant - that's what looks wrong here? Richard. > Now, LTO adds stuff to these chains in lto_fixup_prevailing_type but > doesn't guarantee that. The testcase in the PR (which I'm not including > for testsuite because when (I hope) the aarch64 backend bug will be fixed, > the testcase would work either way) shows a case where user has > TYPE_USER_ALIGN type with very high alignment, as there aren't enough > pointers to float in the code left that one becomes the prevailing one, > lto_fixup_prevailing_type puts it into the TYPE_POINTER_TO chain of > float and later on during expansion of __builtin_cexpif expander > uses build_pointer_type (float_type_node) to emit a sincosf call and > instead of getting a normal pointer type gets this non-standard one. > > The following patch fixes that by not adding into those chains > qualified or user aligned types and by making sure that some type > for the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL combination (e.g. from lto1 > initialization) isn't there already before adding a new one. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2023-02-28 Jakub Jelinek <jakub@redhat.com> > > PR target/108910 > * lto-common.cc (lto_fixup_prevailing_type): Don't add t to > TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has non-zero > TYPE_QUALS, or TYPE_USER_ALIGN or some other type with the > same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already > present. > > --- gcc/lto/lto-common.cc.jj 2023-01-16 11:52:16.165732856 +0100 > +++ gcc/lto/lto-common.cc 2023-02-28 01:42:51.006764018 +0100 > @@ -984,21 +984,35 @@ lto_fixup_prevailing_type (tree t) > TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv); > TYPE_NEXT_VARIANT (mv) = t; > } > - > - /* The following reconstructs the pointer chains > - of the new pointed-to type if we are a main variant. We do > - not stream those so they are broken before fixup. */ > - if (TREE_CODE (t) == POINTER_TYPE > - && TYPE_MAIN_VARIANT (t) == t) > - { > - TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); > - TYPE_POINTER_TO (TREE_TYPE (t)) = t; > - } > - else if (TREE_CODE (t) == REFERENCE_TYPE > - && TYPE_MAIN_VARIANT (t) == t) > + else if (!TYPE_QUALS (t) && !TYPE_USER_ALIGN (t)) > { > - TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); > - TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; > + /* The following reconstructs the pointer chains > + of the new pointed-to type if we are a main variant. We do > + not stream those so they are broken before fixup. > + Don't add it if despite being main variant it is > + qualified or user aligned type. Don't add it if there > + is something in the chain already. */ > + tree *p = NULL; > + if (TREE_CODE (t) == POINTER_TYPE) > + p = &TYPE_POINTER_TO (TREE_TYPE (t)); > + else if (TREE_CODE (t) == REFERENCE_TYPE) > + p = &TYPE_REFERENCE_TO (TREE_TYPE (t)); > + if (p) > + { > + tree t2; > + for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2)) > + if (TYPE_MODE (t2) == TYPE_MODE (t) > + && TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t)) > + break; > + if (t2 == NULL_TREE) > + { > + if (TREE_CODE (t) == POINTER_TYPE) > + TYPE_NEXT_PTR_TO (t) = *p; > + else > + TYPE_NEXT_REF_TO (t) = *p; > + *p = t; > + } > + } > } > } > > > Jakub > >
On Tue, Feb 28, 2023 at 08:58:20AM +0000, Richard Biener wrote: > > Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained > > inside of build_{pointer,reference}_type_for_mode and those routines > > ensure that the pointer/reference type added to the chain is really > > unqualified (including address space), without extra user alignment > > and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL > > pair (unless something would modify the types in place, but that would > > be wrong). > > Is that so? I can't find any code verifying that (verify_type?). > Of course build_{pointer,reference}_type_for_mode will always build > unqualified pointers, but then the LTO code does > > /* The following reconstructs the pointer chains > of the new pointed-to type if we are a main variant. We do > not stream those so they are broken before fixup. */ > if (TREE_CODE (t) == POINTER_TYPE > && TYPE_MAIN_VARIANT (t) == t) > { > TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); > TYPE_POINTER_TO (TREE_TYPE (t)) = t; > } > else if (TREE_CODE (t) == REFERENCE_TYPE > && TYPE_MAIN_VARIANT (t) == t) > { > TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); > TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; > } > > which was supposed to ensure only putting unqualified pointers > (not pointed to types!) to the chain. So to me the question is > rather why a type with TYPE_USER_ALIGN is a the main variant - that's > what looks wrong here? First of all, it is unclear how the above ensures that type isn't added to those chains even when it already is there (or some similar type). Say lto1 during initialization needs build_pointer_type (float_type_node) and later on lto_fixup_prevailing_type is called on some float *. I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT though... Jakub
On Tue, 28 Feb 2023, Jakub Jelinek wrote: > On Tue, Feb 28, 2023 at 08:58:20AM +0000, Richard Biener wrote: > > > Without LTO, TYPE_POINTER_TO/TYPE_REFERENCE_TO chains are only maintained > > > inside of build_{pointer,reference}_type_for_mode and those routines > > > ensure that the pointer/reference type added to the chain is really > > > unqualified (including address space), without extra user alignment > > > and has just one entry for each of the TYPE_MODE/TYPE_REF_CAN_ALIAS_ALL > > > pair (unless something would modify the types in place, but that would > > > be wrong). > > > > Is that so? I can't find any code verifying that (verify_type?). > > Of course build_{pointer,reference}_type_for_mode will always build > > unqualified pointers, but then the LTO code does > > > > /* The following reconstructs the pointer chains > > of the new pointed-to type if we are a main variant. We do > > not stream those so they are broken before fixup. */ > > if (TREE_CODE (t) == POINTER_TYPE > > && TYPE_MAIN_VARIANT (t) == t) > > { > > TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); > > TYPE_POINTER_TO (TREE_TYPE (t)) = t; > > } > > else if (TREE_CODE (t) == REFERENCE_TYPE > > && TYPE_MAIN_VARIANT (t) == t) > > { > > TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); > > TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; > > } > > > > which was supposed to ensure only putting unqualified pointers > > (not pointed to types!) to the chain. So to me the question is > > rather why a type with TYPE_USER_ALIGN is a the main variant - that's > > what looks wrong here? > > First of all, it is unclear how the above ensures that type isn't > added to those chains even when it already is there (or some similar type). > Say lto1 during initialization needs build_pointer_type (float_type_node) > and later on lto_fixup_prevailing_type is called on some float *. We try to make sure to put all built types into the type merging machinery, so I think it shouldn't happen - but then I cannot rule it out. I'm also not sure whether duplicates would really pose a problem here (other than waste). If we want to make sure we should probably enhance verify_types to verify the TYPE_POINTER_TO chain ... > I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT > though... > > Jakub > >
On Tue, Feb 28, 2023 at 09:43:37AM +0000, Richard Biener wrote: > We try to make sure to put all built types into the type merging > machinery, so I think it shouldn't happen - but then I cannot rule > it out. I'm also not sure whether duplicates would really pose > a problem here (other than waste). If we want to make sure we should > probably enhance verify_types to verify the TYPE_POINTER_TO chain ... > > > I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT > > though... Ok, so this happens already in the FEs when trying to add the attribute: #0 build_distinct_type_copy (type=<pointer_type 0x7fffea0219d8>) at ../../gcc/tree.cc:5735 #1 0x0000000000c2327b in build_type_attribute_qual_variant (otype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>, quals=0) at ../../gcc/attribs.cc:1298 #2 0x0000000000c245b0 in build_type_attribute_variant (ttype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>) at ../../gcc/attribs.cc:1591 #3 0x0000000000c22325 in decl_attributes (node=0x7fffffffd008, attributes=<tree_list 0x7fffea01e910>, flags=1, last_decl=<tree 0x0>) at ../../gcc/attribs.cc:964 So, perhaps the !TYPE_QUALS (t) could be just an assert, but maybe next to the !TYPE_USER_ALIGN (t) (or just instead of?) we need !TYPE_ATTRIBUTES (t). Because addition of attributes (but anything else that causes build_distinct_type_copy rather than build_variant_type_copy) will create new TYPE_MAIN_VARIANTS. Looking around, TYPE_REF_IS_RVALUE references also create distinct types, and while the C++ FE sticks them into the TYPE_REFERENCE_TO chain, it ensures they go after the corresponding !TYPE_REF_IS_RVALUE entry, so perhaps LTO should !TYPE_REF_IS_RVALUE for REFERENCE_TYPEs. Other uses of build_distinct_type_copy in the FEs are mostly related to ARRAY_TYPEs (in C FE as well as c-common). Asan uses it solely on integral types, etc. For attributes, big question is if it when we set *no_addr_attrs = true we still tweak some things on the type (not in place) or not. So, here are two possible variant patches which fix the ICE on the testcase too. 2023-02-28 Jakub Jelinek <jakub@redhat.com> PR target/108910 * lto-common.cc (lto_fixup_prevailing_type): Don't add t to TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has TYPE_ATTRIBUTES, or is TYPE_REF_IS_RVALUE, or some other type with the same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already present. --- gcc/lto/lto-common.cc.jj 2023-01-16 11:52:16.165732856 +0100 +++ gcc/lto/lto-common.cc 2023-02-28 12:30:37.014471255 +0100 @@ -984,21 +984,36 @@ lto_fixup_prevailing_type (tree t) TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv); TYPE_NEXT_VARIANT (mv) = t; } - - /* The following reconstructs the pointer chains - of the new pointed-to type if we are a main variant. We do - not stream those so they are broken before fixup. */ - if (TREE_CODE (t) == POINTER_TYPE - && TYPE_MAIN_VARIANT (t) == t) - { - TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); - TYPE_POINTER_TO (TREE_TYPE (t)) = t; - } - else if (TREE_CODE (t) == REFERENCE_TYPE - && TYPE_MAIN_VARIANT (t) == t) + else if (!TYPE_ATTRIBUTES (t)) { - TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); - TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; + /* The following reconstructs the pointer chains + of the new pointed-to type if we are a main variant. We do + not stream those so they are broken before fixup. + Don't add it if despite being main variant it has + attributes (then it was created with build_distinct_type_copy). + Similarly don't add TYPE_REF_IS_RVALUE REFERENCE_TYPEs. + Don't add it if there is something in the chain already. */ + tree *p = NULL; + if (TREE_CODE (t) == POINTER_TYPE) + p = &TYPE_POINTER_TO (TREE_TYPE (t)); + else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t)) + p = &TYPE_REFERENCE_TO (TREE_TYPE (t)); + if (p) + { + tree t2; + for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2)) + if (TYPE_MODE (t2) == TYPE_MODE (t) + && TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t)) + break; + if (t2 == NULL_TREE) + { + if (TREE_CODE (t) == POINTER_TYPE) + TYPE_NEXT_PTR_TO (t) = *p; + else + TYPE_NEXT_REF_TO (t) = *p; + *p = t; + } + } } } Jakub 2023-02-28 Jakub Jelinek <jakub@redhat.com> PR target/108910 * lto-common.cc (lto_fixup_prevailing_type): Don't add t to TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has TYPE_ATTRIBUTES or is TYPE_REF_IS_RVALUE. --- gcc/lto/lto-common.cc.jj 2023-01-16 11:52:16.165732856 +0100 +++ gcc/lto/lto-common.cc 2023-02-28 12:34:33.698038478 +0100 @@ -984,21 +984,25 @@ lto_fixup_prevailing_type (tree t) TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv); TYPE_NEXT_VARIANT (mv) = t; } - - /* The following reconstructs the pointer chains - of the new pointed-to type if we are a main variant. We do - not stream those so they are broken before fixup. */ - if (TREE_CODE (t) == POINTER_TYPE - && TYPE_MAIN_VARIANT (t) == t) - { - TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); - TYPE_POINTER_TO (TREE_TYPE (t)) = t; - } - else if (TREE_CODE (t) == REFERENCE_TYPE - && TYPE_MAIN_VARIANT (t) == t) + else if (!TYPE_ATTRIBUTES (t)) { - TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); - TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; + /* The following reconstructs the pointer chains + of the new pointed-to type if we are a main variant. We do + not stream those so they are broken before fixup. + Don't add it if despite being main variant it has + attributes (then it was created with build_distinct_type_copy). + Similarly don't add TYPE_REF_IS_RVALUE REFERENCE_TYPEs. + Don't add it if there is something in the chain already. */ + if (TREE_CODE (t) == POINTER_TYPE) + { + TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); + TYPE_POINTER_TO (TREE_TYPE (t)) = t; + } + else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t)) + { + TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); + TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; + } } }
On Tue, 28 Feb 2023, Jakub Jelinek wrote: > On Tue, Feb 28, 2023 at 09:43:37AM +0000, Richard Biener wrote: > > We try to make sure to put all built types into the type merging > > machinery, so I think it shouldn't happen - but then I cannot rule > > it out. I'm also not sure whether duplicates would really pose > > a problem here (other than waste). If we want to make sure we should > > probably enhance verify_types to verify the TYPE_POINTER_TO chain ... > > > > > I'll try to debug why those user aligned types become TYPE_MAIN_VARIANT > > > though... > > Ok, so this happens already in the FEs when trying to add the attribute: > #0 build_distinct_type_copy (type=<pointer_type 0x7fffea0219d8>) at ../../gcc/tree.cc:5735 > #1 0x0000000000c2327b in build_type_attribute_qual_variant (otype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>, quals=0) at ../../gcc/attribs.cc:1298 > #2 0x0000000000c245b0 in build_type_attribute_variant (ttype=<pointer_type 0x7fffea0219d8>, attribute=<tree_list 0x7fffea01e938>) at ../../gcc/attribs.cc:1591 > #3 0x0000000000c22325 in decl_attributes (node=0x7fffffffd008, attributes=<tree_list 0x7fffea01e910>, flags=1, last_decl=<tree 0x0>) at ../../gcc/attribs.cc:964 > > So, perhaps the !TYPE_QUALS (t) could be just an assert, but maybe next to > the !TYPE_USER_ALIGN (t) (or just instead of?) we need !TYPE_ATTRIBUTES (t). > Because addition of attributes (but anything else that causes > build_distinct_type_copy rather than build_variant_type_copy) will create > new TYPE_MAIN_VARIANTS. > Looking around, TYPE_REF_IS_RVALUE references also create distinct types, > and while the C++ FE sticks them into the TYPE_REFERENCE_TO chain, it > ensures they go after the corresponding !TYPE_REF_IS_RVALUE entry, so > perhaps LTO should !TYPE_REF_IS_RVALUE for REFERENCE_TYPEs. > Other uses of build_distinct_type_copy in the FEs are mostly related to > ARRAY_TYPEs (in C FE as well as c-common). Asan uses it solely on integral > types, etc. For attributes, big question is if it when we set > *no_addr_attrs = true we still tweak some things on the type (not in place) > or not. > > So, here are two possible variant patches which fix the ICE on the > testcase too. > > 2023-02-28 Jakub Jelinek <jakub@redhat.com> > > PR target/108910 > * lto-common.cc (lto_fixup_prevailing_type): Don't add t to > TYPE_POINTER_TO or TYPE_REFERENCE_TO chain if it has > TYPE_ATTRIBUTES, or is TYPE_REF_IS_RVALUE, or some other type > with the same TYPE_MODE and TYPE_REF_CAN_ALIAS_ALL flag is already > present. > > --- gcc/lto/lto-common.cc.jj 2023-01-16 11:52:16.165732856 +0100 > +++ gcc/lto/lto-common.cc 2023-02-28 12:30:37.014471255 +0100 > @@ -984,21 +984,36 @@ lto_fixup_prevailing_type (tree t) > TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv); > TYPE_NEXT_VARIANT (mv) = t; > } > - > - /* The following reconstructs the pointer chains > - of the new pointed-to type if we are a main variant. We do > - not stream those so they are broken before fixup. */ > - if (TREE_CODE (t) == POINTER_TYPE > - && TYPE_MAIN_VARIANT (t) == t) > - { > - TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); > - TYPE_POINTER_TO (TREE_TYPE (t)) = t; > - } > - else if (TREE_CODE (t) == REFERENCE_TYPE > - && TYPE_MAIN_VARIANT (t) == t) > + else if (!TYPE_ATTRIBUTES (t)) > { > - TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); > - TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; > + /* The following reconstructs the pointer chains > + of the new pointed-to type if we are a main variant. We do > + not stream those so they are broken before fixup. > + Don't add it if despite being main variant it has > + attributes (then it was created with build_distinct_type_copy). > + Similarly don't add TYPE_REF_IS_RVALUE REFERENCE_TYPEs. > + Don't add it if there is something in the chain already. */ > + tree *p = NULL; > + if (TREE_CODE (t) == POINTER_TYPE) > + p = &TYPE_POINTER_TO (TREE_TYPE (t)); > + else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t)) > + p = &TYPE_REFERENCE_TO (TREE_TYPE (t)); > + if (p) > + { > + tree t2; > + for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2)) > + if (TYPE_MODE (t2) == TYPE_MODE (t) > + && TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t)) > + break; Can we elide this searching please? Having duplicated should be harmless unless proved otherwise. OK with that change. Richard. > + if (t2 == NULL_TREE) > + { > + if (TREE_CODE (t) == POINTER_TYPE) > + TYPE_NEXT_PTR_TO (t) = *p; > + else > + TYPE_NEXT_REF_TO (t) = *p; > + *p = t; > + } > + } > } > } > > > > Jakub >
On Tue, Feb 28, 2023 at 12:05:20PM +0000, Richard Biener via Gcc-patches wrote: > > + p = &TYPE_POINTER_TO (TREE_TYPE (t)); > > + else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t)) > > + p = &TYPE_REFERENCE_TO (TREE_TYPE (t)); > > + if (p) > > + { > > + tree t2; > > + for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2)) > > + if (TYPE_MODE (t2) == TYPE_MODE (t) > > + && TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t)) > > + break; > > Can we elide this searching please? Having duplicated should be harmless > unless proved otherwise. I've posted 2 patches (one inlined, another attached), the second one didn't do this at all. Having (too many) duplicates would be harmful because build_pointer_type etc. walk up to the whole length of list all the time. When the list length is bounded (say at most 2 modes - ptr_mode/Pmode, times 2 (the can alias all bool), then it doesn't hurt, if it could in theory be arbitrarily long, it would be a compile time problem. But given that we with the !TYPE_ATTRIBUTES/!TYPE_REF_IS_RVALUE change don't have a testcase that would show it is a problem actually ever encounterable, the second patch without this is fine I think. > > OK with that change. Jakub
On Tue, 28 Feb 2023, Jakub Jelinek wrote: > On Tue, Feb 28, 2023 at 12:05:20PM +0000, Richard Biener via Gcc-patches wrote: > > > + p = &TYPE_POINTER_TO (TREE_TYPE (t)); > > > + else if (TREE_CODE (t) == REFERENCE_TYPE && !TYPE_REF_IS_RVALUE (t)) > > > + p = &TYPE_REFERENCE_TO (TREE_TYPE (t)); > > > + if (p) > > > + { > > > + tree t2; > > > + for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2)) > > > + if (TYPE_MODE (t2) == TYPE_MODE (t) > > > + && TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t)) > > > + break; > > > > Can we elide this searching please? Having duplicated should be harmless > > unless proved otherwise. > > I've posted 2 patches (one inlined, another attached), the second one > didn't do this at all. Ah, didn't notice that. > Having (too many) duplicates would be harmful because build_pointer_type > etc. walk up to the whole length of list all the time. When the list > length is bounded (say at most 2 modes - ptr_mode/Pmode, times 2 (the > can alias all bool), then it doesn't hurt, if it could in theory be > arbitrarily long, it would be a compile time problem. > But given that we with the !TYPE_ATTRIBUTES/!TYPE_REF_IS_RVALUE change > don't have a testcase that would show it is a problem actually ever > encounterable, the second patch without this is fine I think. I agree the attached patch is fine. Richard.
--- gcc/lto/lto-common.cc.jj 2023-01-16 11:52:16.165732856 +0100 +++ gcc/lto/lto-common.cc 2023-02-28 01:42:51.006764018 +0100 @@ -984,21 +984,35 @@ lto_fixup_prevailing_type (tree t) TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv); TYPE_NEXT_VARIANT (mv) = t; } - - /* The following reconstructs the pointer chains - of the new pointed-to type if we are a main variant. We do - not stream those so they are broken before fixup. */ - if (TREE_CODE (t) == POINTER_TYPE - && TYPE_MAIN_VARIANT (t) == t) - { - TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (TREE_TYPE (t)); - TYPE_POINTER_TO (TREE_TYPE (t)) = t; - } - else if (TREE_CODE (t) == REFERENCE_TYPE - && TYPE_MAIN_VARIANT (t) == t) + else if (!TYPE_QUALS (t) && !TYPE_USER_ALIGN (t)) { - TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (TREE_TYPE (t)); - TYPE_REFERENCE_TO (TREE_TYPE (t)) = t; + /* The following reconstructs the pointer chains + of the new pointed-to type if we are a main variant. We do + not stream those so they are broken before fixup. + Don't add it if despite being main variant it is + qualified or user aligned type. Don't add it if there + is something in the chain already. */ + tree *p = NULL; + if (TREE_CODE (t) == POINTER_TYPE) + p = &TYPE_POINTER_TO (TREE_TYPE (t)); + else if (TREE_CODE (t) == REFERENCE_TYPE) + p = &TYPE_REFERENCE_TO (TREE_TYPE (t)); + if (p) + { + tree t2; + for (t2 = *p; t2; t2 = TYPE_NEXT_PTR_TO (t2)) + if (TYPE_MODE (t2) == TYPE_MODE (t) + && TYPE_REF_CAN_ALIAS_ALL (t2) == TYPE_REF_CAN_ALIAS_ALL (t)) + break; + if (t2 == NULL_TREE) + { + if (TREE_CODE (t) == POINTER_TYPE) + TYPE_NEXT_PTR_TO (t) = *p; + else + TYPE_NEXT_REF_TO (t) = *p; + *p = t; + } + } } }