Message ID | 1cf184b6-a0a1-7514-37e1-ddd8c6b7d7e3@suse.cz |
---|---|
State | Committed |
Commit | 31ce821a790caec8a2849dd67a9847e78a33d14c |
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 7CE5B38515DA for <patchwork@sourceware.org>; Thu, 19 May 2022 07:09:05 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 9B4053851C02 for <gcc-patches@gcc.gnu.org>; Thu, 19 May 2022 07:08:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9B4053851C02 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 50C6921C2E; Thu, 19 May 2022 07:08:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1652944128; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=K9JT5L6l2+1SWiEM2QjW5BMDG/hizahmKqd2PyZUzY0=; b=isecS1M8iiQV93XJ69sktEIPqqwSgM4fDzpxDyXOxl56TSTkrcGOXi9S5U8mebi4BeHOOA QIDX4bXSg9jaDDkurPEYs056LVZvYI9H0mtjZxkou8YjQx85haYdCwrMJwJFi4KwetUOht K2VFOnqwtgT0DD3s4amzJd5UBPL5W30= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1652944128; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=K9JT5L6l2+1SWiEM2QjW5BMDG/hizahmKqd2PyZUzY0=; b=4FKZPYbI7NXLQn23DsTYbEPUmeRlz9IPpnH4uwC+bKst/sRyLNRRtWg8wAZuKu/Ni9kaq/ eq4dMmjuHo0V66DQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 3487113456; Thu, 19 May 2022 07:08:48 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id hCl7CwDthWI+EwAAMHmgww (envelope-from <mliska@suse.cz>); Thu, 19 May 2022 07:08:48 +0000 Message-ID: <1cf184b6-a0a1-7514-37e1-ddd8c6b7d7e3@suse.cz> Date: Thu, 19 May 2022 09:08:47 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH] ipa-icf: skip variables with body_removed To: gcc-patches@gcc.gnu.org Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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> Cc: Jan Hubicka <hubicka@ucw.cz> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
ipa-icf: skip variables with body_removed
|
|
Commit Message
Martin Liška
May 19, 2022, 7:08 a.m. UTC
Similarly to cgraph_nodes, it may happen that body_removed is set during merging of symbols. PR ipa/105600 Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: * ipa-icf.cc (sem_item_optimizer::filter_removed_items): Skip variables with body_removed. --- gcc/ipa-icf.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Comments
> Similarly to cgraph_nodes, it may happen that body_removed is set > during merging of symbols. > > PR ipa/105600 > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > * ipa-icf.cc (sem_item_optimizer::filter_removed_items): > Skip variables with body_removed. > --- > gcc/ipa-icf.cc | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc > index 765ae746745..6528a7a10b2 100644 > --- a/gcc/ipa-icf.cc > +++ b/gcc/ipa-icf.cc > @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void) > { > /* Filter out non-readonly variables. */ > tree decl = item->decl; > - if (TREE_READONLY (decl)) > - filtered.safe_push (item); > - else > + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node (); > + if (!TREE_READONLY (decl) || vnode->body_removed) > remove_item (item); > + else > + filtered.safe_push (item); So the situation here is that we merge symbols but keep syntactic alias because the declarations are not compatible (have different attributes perhaps because of fortify source)? Will ICF still see through the aliases and do merging? I think you can craft a testcase by triggering the attribute mismatch and see what happens. At the time we implemented ICF these aliases did not exists, so maybe some TLC is needed here. Honza
On 5/19/22 17:02, Jan Hubicka wrote: >> Similarly to cgraph_nodes, it may happen that body_removed is set >> during merging of symbols. >> >> PR ipa/105600 >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> * ipa-icf.cc (sem_item_optimizer::filter_removed_items): >> Skip variables with body_removed. >> --- >> gcc/ipa-icf.cc | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc >> index 765ae746745..6528a7a10b2 100644 >> --- a/gcc/ipa-icf.cc >> +++ b/gcc/ipa-icf.cc >> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void) >> { >> /* Filter out non-readonly variables. */ >> tree decl = item->decl; >> - if (TREE_READONLY (decl)) >> - filtered.safe_push (item); >> - else >> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node (); >> + if (!TREE_READONLY (decl) || vnode->body_removed) >> remove_item (item); >> + else >> + filtered.safe_push (item); > > So the situation here is that we merge symbols but keep syntactic alias > because the declarations are not compatible (have different attributes > perhaps because of fortify source)? The test-case is more about a constexpr symbol or so, I'm not familiar enough with these modern C++. cgraph_node looks like: (gdb) p item->node->debug() _ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE/10 (value) @0x7ffff7fb3200 Type: variable Body removed by symtab_remove_unreachable_nodes Visibility: externally_visible semantic_interposition preempted_reg external public weak comdat comdat_group:_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE one_only References: Referring: _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) Read from file: a.o Availability: not_available Varpool flags: initialized read-only > > Will ICF still see through the aliases and do merging? No. > I think you can > craft a testcase by triggering the attribute mismatch and see what > happens. At the time we implemented ICF these aliases did not exists, > so maybe some TLC is needed here. Please come up with such test case :) Thanks, Martin > > Honza
@Honza: PING On 5/20/22 09:46, Martin Liška wrote: > On 5/19/22 17:02, Jan Hubicka wrote: >>> Similarly to cgraph_nodes, it may happen that body_removed is set >>> during merging of symbols. >>> >>> PR ipa/105600 >>> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>> >>> Ready to be installed? >>> Thanks, >>> Martin >>> >>> gcc/ChangeLog: >>> >>> * ipa-icf.cc (sem_item_optimizer::filter_removed_items): >>> Skip variables with body_removed. >>> --- >>> gcc/ipa-icf.cc | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc >>> index 765ae746745..6528a7a10b2 100644 >>> --- a/gcc/ipa-icf.cc >>> +++ b/gcc/ipa-icf.cc >>> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void) >>> { >>> /* Filter out non-readonly variables. */ >>> tree decl = item->decl; >>> - if (TREE_READONLY (decl)) >>> - filtered.safe_push (item); >>> - else >>> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node (); >>> + if (!TREE_READONLY (decl) || vnode->body_removed) >>> remove_item (item); >>> + else >>> + filtered.safe_push (item); >> >> So the situation here is that we merge symbols but keep syntactic alias >> because the declarations are not compatible (have different attributes >> perhaps because of fortify source)? > > The test-case is more about a constexpr symbol or so, I'm not familiar enough > with these modern C++. > > cgraph_node looks like: > > (gdb) p item->node->debug() > _ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE/10 (value) @0x7ffff7fb3200 > Type: variable > Body removed by symtab_remove_unreachable_nodes > Visibility: externally_visible semantic_interposition preempted_reg external public weak comdat comdat_group:_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE one_only > References: > Referring: _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) > Read from file: a.o > Availability: not_available > Varpool flags: initialized read-only > > >> >> Will ICF still see through the aliases and do merging? > > No. > >> I think you can >> craft a testcase by triggering the attribute mismatch and see what >> happens. At the time we implemented ICF these aliases did not exists, >> so maybe some TLC is needed here. > > Please come up with such test case :) > > Thanks, > Martin > >> >> Honza >
> @Honza: PING > > On 5/20/22 09:46, Martin Liška wrote: > > On 5/19/22 17:02, Jan Hubicka wrote: > >>> Similarly to cgraph_nodes, it may happen that body_removed is set > >>> during merging of symbols. > >>> > >>> PR ipa/105600 > >>> > >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >>> > >>> Ready to be installed? > >>> Thanks, > >>> Martin > >>> > >>> gcc/ChangeLog: > >>> > >>> * ipa-icf.cc (sem_item_optimizer::filter_removed_items): > >>> Skip variables with body_removed. > >>> --- > >>> gcc/ipa-icf.cc | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc > >>> index 765ae746745..6528a7a10b2 100644 > >>> --- a/gcc/ipa-icf.cc > >>> +++ b/gcc/ipa-icf.cc > >>> @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void) > >>> { > >>> /* Filter out non-readonly variables. */ > >>> tree decl = item->decl; > >>> - if (TREE_READONLY (decl)) > >>> - filtered.safe_push (item); > >>> - else > >>> + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node (); > >>> + if (!TREE_READONLY (decl) || vnode->body_removed) > >>> remove_item (item); > >>> + else > >>> + filtered.safe_push (item); > >> > >> So the situation here is that we merge symbols but keep syntactic alias > >> because the declarations are not compatible (have different attributes > >> perhaps because of fortify source)? > > > > The test-case is more about a constexpr symbol or so, I'm not familiar enough > > with these modern C++. > > > > cgraph_node looks like: > > > > (gdb) p item->node->debug() > > _ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE/10 (value) @0x7ffff7fb3200 > > Type: variable > > Body removed by symtab_remove_unreachable_nodes > > Visibility: externally_visible semantic_interposition preempted_reg external public weak comdat comdat_group:_ZN8nlohmann6detail12static_constINS0_10to_json_fnEE5valueE one_only > > References: > > Referring: _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) _Z7to_jsonRN8nlohmann10basic_jsonISt3mapSt6vectorNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEblmiSaNS_14adl_serializerES2_IhSaIhEEEERK8Settings/1 (addr) > > Read from file: a.o > > Availability: not_available > > Varpool flags: initialized read-only OK, I think I worked out what happens here. The thing is that the symbol has "external" flag set. In my mental picture we should be keeping their definitions around since they are used for constant folding, but it seems it is not what happens. We special case inline functions but we do not special case external symbols. This is a missed optimization oppurtunity but not completely trivial to fix, since external symbol may reffer to internal one (such as comdat) but even if it does so we want to optimize out that comdat. So we need some bookkeeping for referneces from external symbols. I will implement that but I guess we still want your patch since this is optional optimization. So patch is OK. Honza > > > > > >> > >> Will ICF still see through the aliases and do merging? > > > > No. > > > >> I think you can > >> craft a testcase by triggering the attribute mismatch and see what > >> happens. At the time we implemented ICF these aliases did not exists, > >> so maybe some TLC is needed here. > > > > Please come up with such test case :) > > > > Thanks, > > Martin > > > >> > >> Honza > > >
diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc index 765ae746745..6528a7a10b2 100644 --- a/gcc/ipa-icf.cc +++ b/gcc/ipa-icf.cc @@ -2411,10 +2411,11 @@ sem_item_optimizer::filter_removed_items (void) { /* Filter out non-readonly variables. */ tree decl = item->decl; - if (TREE_READONLY (decl)) - filtered.safe_push (item); - else + varpool_node *vnode = static_cast <sem_variable *>(item)->get_node (); + if (!TREE_READONLY (decl) || vnode->body_removed) remove_item (item); + else + filtered.safe_push (item); } } }