Message ID | a4208860-1af9-6c47-6109-5c2fe4c9d444@suse.cz |
---|---|
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 348A2385841C for <patchwork@sourceware.org>; Thu, 1 Dec 2022 09:59:26 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id D3E463858D3C for <gcc-patches@gcc.gnu.org>; Thu, 1 Dec 2022 09:59:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D3E463858D3C 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 imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (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-out2.suse.de (Postfix) with ESMTPS id 8DB961FD68; Thu, 1 Dec 2022 09:59:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1669888748; 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=rnao0Mvqu/X21Jt381uphZoQCH7Tn0dP5UXkgMf9VYU=; b=b9BLZw18eBt5Pel1OmLM6doRUCLqsoBEuotF2KzuDW+8Qxn3YDbFw3S1xnta7+XgzUu3gn rbX9T5d38n5sTKAFhyHxKUQ8YjlVCAEBYFqALOeqQMCaL+bOf3rDdQ2rNyldlPIyPTWfSE ZGzpYzgAwDfhHnwzUQ94XHE9Xdqzisg= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1669888748; 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=rnao0Mvqu/X21Jt381uphZoQCH7Tn0dP5UXkgMf9VYU=; b=6Ku8Mla48i6UOQfx8d/P/+Tg5zKQitM8HLedR5x2crqEtJ7wXCL/DX73ilhoZckUbsHnss CcjZy8TXvFqYxVBg== Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (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 imap1.suse-dmz.suse.de (Postfix) with ESMTPS id 6C1241320E; Thu, 1 Dec 2022 09:59:08 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap1.suse-dmz.suse.de with ESMTPSA id 3YBaGex6iGMBHQAAGKfGzw (envelope-from <mliska@suse.cz>); Thu, 01 Dec 2022 09:59:08 +0000 Message-ID: <a4208860-1af9-6c47-6109-5c2fe4c9d444@suse.cz> Date: Thu, 1 Dec 2022 10:59:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH] IPA: do not release body if still needed To: gcc-patches@gcc.gnu.org Content-Language: en-US Cc: Martin Jambor <mjambor@suse.cz> 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 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> 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: do not release body if still needed
|
|
Commit Message
Martin Liška
Dec. 1, 2022, 9:59 a.m. UTC
Hi. Noticed during building of libbackend.a with the LTO partial linking. The function release_body is called even if clone_of is a clone of a another function and thus it shares tree declaration. We should preserve it in that situation. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin PR ipa/107944 gcc/ChangeLog: * cgraph.cc (cgraph_node::remove): Do not release body if a node is clone of another node. --- gcc/cgraph.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
PING^1 On 12/1/22 10:59, Martin Liška wrote: > Hi. > > Noticed during building of libbackend.a with the LTO partial linking. > > The function release_body is called even if clone_of is a clone > of a another function and thus it shares tree declaration. We should > preserve it in that situation. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > PR ipa/107944 > > gcc/ChangeLog: > > * cgraph.cc (cgraph_node::remove): Do not release body > if a node is clone of another node. > --- > gcc/cgraph.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc > index f15cb47c8b8..2e7d77ffd6c 100644 > --- a/gcc/cgraph.cc > +++ b/gcc/cgraph.cc > @@ -1893,7 +1893,7 @@ cgraph_node::remove (void) > else if (clone_of) > { > clone_of->clones = next_sibling_clone; > - if (!clone_of->analyzed && !clone_of->clones && !clones) > + if (!clone_of->analyzed && !clone_of->clones && !clones && !clone_of->clone_of) > clone_of->release_body (); > } > if (next_sibling_clone)
PING^2 On 12/9/22 09:28, Martin Liška wrote: > PING^1 > > On 12/1/22 10:59, Martin Liška wrote: >> Hi. >> >> Noticed during building of libbackend.a with the LTO partial linking. >> >> The function release_body is called even if clone_of is a clone >> of a another function and thus it shares tree declaration. We should >> preserve it in that situation. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> >> PR ipa/107944 >> >> gcc/ChangeLog: >> >> * cgraph.cc (cgraph_node::remove): Do not release body >> if a node is clone of another node. >> --- >> gcc/cgraph.cc | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc >> index f15cb47c8b8..2e7d77ffd6c 100644 >> --- a/gcc/cgraph.cc >> +++ b/gcc/cgraph.cc >> @@ -1893,7 +1893,7 @@ cgraph_node::remove (void) >> else if (clone_of) >> { >> clone_of->clones = next_sibling_clone; >> - if (!clone_of->analyzed && !clone_of->clones && !clones) >> + if (!clone_of->analyzed && !clone_of->clones && !clones && !clone_of->clone_of) >> clone_of->release_body (); >> } >> if (next_sibling_clone) >
Hi, sorry for getting to this so late. On Thu, Dec 01 2022, Martin Liška wrote: > Hi. > > Noticed during building of libbackend.a with the LTO partial linking. The testcase is areally nice one, too bad it's probably impossible to get it small enough to be included in the testcase. But it also fails to fail for me on trunk, I could only reproduce the problem on the gcc-12 branch. > > The function release_body is called even if clone_of is a clone > of a another function and thus it shares tree declaration. We should > preserve it in that situation. > But then PR 100413 could happen just one level higher in the clones hierarchy, not for clone_of but for clone_of->clone_of, no? I think we need something like the following (only lightly tested so far, I'll bootstrap it over the weekend): diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc index 4bb9e7ba6af..3734c85db63 100644 --- a/gcc/cgraph.cc +++ b/gcc/cgraph.cc @@ -1895,8 +1895,18 @@ cgraph_node::remove (void) else if (clone_of) { clone_of->clones = next_sibling_clone; - if (!clone_of->analyzed && !clone_of->clones && !clones) - clone_of->release_body (); + if (!clones) + { + bool need_body = false; + for (cgraph_node *n = clone_of; n; n = n->clone_of) + if (n->analyzed || n->clones) + { + need_body = true; + break; + } + if (!need_body) + clone_of->release_body (); + } } if (next_sibling_clone) next_sibling_clone->prev_sibling_clone = prev_sibling_clone; Thanks for catching this. Martin
> Hi. > > Noticed during building of libbackend.a with the LTO partial linking. > > The function release_body is called even if clone_of is a clone > of a another function and thus it shares tree declaration. We should > preserve it in that situation. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > PR ipa/107944 > > gcc/ChangeLog: > > * cgraph.cc (cgraph_node::remove): Do not release body > if a node is clone of another node. > --- > gcc/cgraph.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc > index f15cb47c8b8..2e7d77ffd6c 100644 > --- a/gcc/cgraph.cc > +++ b/gcc/cgraph.cc > @@ -1893,7 +1893,7 @@ cgraph_node::remove (void) > else if (clone_of) > { > clone_of->clones = next_sibling_clone; > - if (!clone_of->analyzed && !clone_of->clones && !clones) > + if (!clone_of->analyzed && !clone_of->clones && !clones && !clone_of->clone_of) > clone_of->release_body (); It is interesting that the problem reproduced only after almost 20 years. But I suppose it is because we materialize clones in parituclar order. I think there are two ways to fix it. Either declare release_body to be applicable only to the master clone and avoid calling it here (as you do) or make release_body do nothing when called on a clone. I guess it makes sense to keep your approach but please add sanity check to release_body that clone_of == NULL with a comment. OK with that change. Honza > } > if (next_sibling_clone) > -- > 2.38.1 >
On 1/14/23 22:36, Jan Hubicka wrote: >> Hi. >> >> Noticed during building of libbackend.a with the LTO partial linking. >> >> The function release_body is called even if clone_of is a clone >> of a another function and thus it shares tree declaration. We should >> preserve it in that situation. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> >> PR ipa/107944 >> >> gcc/ChangeLog: >> >> * cgraph.cc (cgraph_node::remove): Do not release body >> if a node is clone of another node. >> --- >> gcc/cgraph.cc | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc >> index f15cb47c8b8..2e7d77ffd6c 100644 >> --- a/gcc/cgraph.cc >> +++ b/gcc/cgraph.cc >> @@ -1893,7 +1893,7 @@ cgraph_node::remove (void) >> else if (clone_of) >> { >> clone_of->clones = next_sibling_clone; >> - if (!clone_of->analyzed && !clone_of->clones && !clones) >> + if (!clone_of->analyzed && !clone_of->clones && !clones && !clone_of->clone_of) >> clone_of->release_body (); > > It is interesting that the problem reproduced only after almost 20 > years. But I suppose it is because we materialize clones in parituclar > order. Well, it started with r13-48-g27ee75dbe81bb7 where Martin add a new code that calls the release_body function. So it's pretty new. > > I think there are two ways to fix it. Either declare release_body to be > applicable only to the master clone and avoid calling it here (as you > do) or make release_body do nothing when called on a clone. > I guess it makes sense to keep your approach but please add sanity check > to release_body that clone_of == NULL with a comment. I do support Martin's enhanced version of the patch. Cheers, Martin > > OK with that change. > Honza >> } >> if (next_sibling_clone) >> -- >> 2.38.1 >>
Hi, On Mon, Jan 16 2023, Martin Liška wrote: > On 1/14/23 22:36, Jan Hubicka wrote: >>> Noticed during building of libbackend.a with the LTO partial linking. >>> >>> The function release_body is called even if clone_of is a clone >>> of a another function and thus it shares tree declaration. We should >>> preserve it in that situation. >>> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>> >>> Ready to be installed? >>> Thanks, >>> Martin >>> >>> PR ipa/107944 >>> >>> gcc/ChangeLog: >>> >>> * cgraph.cc (cgraph_node::remove): Do not release body >>> if a node is clone of another node. >>> --- >>> gcc/cgraph.cc | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc >>> index f15cb47c8b8..2e7d77ffd6c 100644 >>> --- a/gcc/cgraph.cc >>> +++ b/gcc/cgraph.cc >>> @@ -1893,7 +1893,7 @@ cgraph_node::remove (void) >>> else if (clone_of) >>> { >>> clone_of->clones = next_sibling_clone; >>> - if (!clone_of->analyzed && !clone_of->clones && !clones) >>> + if (!clone_of->analyzed && !clone_of->clones && !clones && !clone_of->clone_of) >>> clone_of->release_body (); >> >> It is interesting that the problem reproduced only after almost 20 >> years. But I suppose it is because we materialize clones in parituclar >> order. > > Well, it started with r13-48-g27ee75dbe81bb7 where Martin add a new code > that calls the release_body function. So it's pretty new. > >> >> I think there are two ways to fix it. Either declare release_body to be >> applicable only to the master clone and avoid calling it here (as you >> do) or make release_body do nothing when called on a clone. >> I guess it makes sense to keep your approach but please add sanity check >> to release_body that clone_of == NULL with a comment. > > I do support Martin's enhanced version of the patch. > I take that as an approval, so I am about to commit the following after re-testing it on trunk. Afterwards I'll backport it to the affected release branches too. Thanks, Martin The code removing function bodies when the last call graph clone of a node is removed is too aggressive when there are nodes up the clone_of chain which still need them. Fixed by expanding the check. gcc/ChangeLog: 2023-01-18 Martin Jambor <mjambor@suse.cz> PR ipa/107944 * cgraph.cc (cgraph_node::remove): Check whether nodes up the lcone_of chain also do not need the body. --- gcc/cgraph.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc index 5e60c2b73db..5f72ace9b57 100644 --- a/gcc/cgraph.cc +++ b/gcc/cgraph.cc @@ -1893,8 +1893,18 @@ cgraph_node::remove (void) else if (clone_of) { clone_of->clones = next_sibling_clone; - if (!clone_of->analyzed && !clone_of->clones && !clones) - clone_of->release_body (); + if (!clones) + { + bool need_body = false; + for (cgraph_node *n = clone_of; n; n = n->clone_of) + if (n->analyzed || n->clones) + { + need_body = true; + break; + } + if (!need_body) + clone_of->release_body (); + } } if (next_sibling_clone) next_sibling_clone->prev_sibling_clone = prev_sibling_clone;
> The code removing function bodies when the last call graph clone of a > node is removed is too aggressive when there are nodes up the > clone_of chain which still need them. Fixed by expanding the check. > > gcc/ChangeLog: > > 2023-01-18 Martin Jambor <mjambor@suse.cz> > > PR ipa/107944 > * cgraph.cc (cgraph_node::remove): Check whether nodes up the > lcone_of chain also do not need the body. > --- > gcc/cgraph.cc | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc > index 5e60c2b73db..5f72ace9b57 100644 > --- a/gcc/cgraph.cc > +++ b/gcc/cgraph.cc > @@ -1893,8 +1893,18 @@ cgraph_node::remove (void) > else if (clone_of) > { > clone_of->clones = next_sibling_clone; > - if (!clone_of->analyzed && !clone_of->clones && !clones) > - clone_of->release_body (); > + if (!clones) > + { > + bool need_body = false; > + for (cgraph_node *n = clone_of; n; n = n->clone_of) > + if (n->analyzed || n->clones) > + { > + need_body = true; If you want to walk immediate clones and see if any of them is needed, I wonder why you don't also walk recursively clones of clones? Original idea was that the clones should be materialized and removed one by one (or proved unreachable and just removed) and once we remove last one, we should figure out that body is not needed. For that one does not not need the walk at all. How exactly we end up with clones that are not analyzed? Honza > + break; > + } > + if (!need_body) > + clone_of->release_body (); > + } > } > if (next_sibling_clone) > next_sibling_clone->prev_sibling_clone = prev_sibling_clone; > -- > 2.39.0 >
Hi, On Wed, Jan 18 2023, Jan Hubicka wrote: >> The code removing function bodies when the last call graph clone of a >> node is removed is too aggressive when there are nodes up the >> clone_of chain which still need them. Fixed by expanding the check. >> >> gcc/ChangeLog: >> >> 2023-01-18 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/107944 >> * cgraph.cc (cgraph_node::remove): Check whether nodes up the >> lcone_of chain also do not need the body. >> --- >> gcc/cgraph.cc | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc >> index 5e60c2b73db..5f72ace9b57 100644 >> --- a/gcc/cgraph.cc >> +++ b/gcc/cgraph.cc >> @@ -1893,8 +1893,18 @@ cgraph_node::remove (void) >> else if (clone_of) >> { >> clone_of->clones = next_sibling_clone; >> - if (!clone_of->analyzed && !clone_of->clones && !clones) >> - clone_of->release_body (); >> + if (!clones) >> + { >> + bool need_body = false; >> + for (cgraph_node *n = clone_of; n; n = n->clone_of) >> + if (n->analyzed || n->clones) >> + { >> + need_body = true; > If you want to walk immediate clones and see if any of them is needed, I > wonder why you don't also walk recursively clones of clones? The intent is to avoid PR 100413. When a node is being removed, we need to figure out if it is the last one needing the body. If a (possibly indirect) clone_of has a clone, they are still to be materialized and so the body is necessary. If those other clones are all also going to be removed as unreachable rather than materialized, then the last one will release the body. > > Original idea was that the clones should be materialized and removed one > by one (or proved unreachable and just removed) and once we remove last > one, we should figure out that body is not needed. For that one does not > not need the walk at all. So if you have clones of F like this F / \ A B / \ C D / \ M R And then A and C are removed as unreachable or materialized, M is materialized, and afterwards R is removed as unreachable then the removal of R also has to trigger releasing the body. In order not to trigger the bug we are fixing, it needs to check that neither of D, B or F need the body themselves or have any clones which need it. Thus the walk. Now, the method as an alternative point where it releases the body a few lines below, and having two looks a bit clumsy. But it is not entirely straightforward how to combine the conditions guarding the two places. > > How exactly we end up with clones that are not analyzed? I hope I am not misremembering but analyzed gets cleared when a node is there just to hold body for its clones and is no longer necessary for any other reason, no? Martin > > Honza >> + break; >> + } >> + if (!need_body) >> + clone_of->release_body (); >> + } >> } >> if (next_sibling_clone) >> next_sibling_clone->prev_sibling_clone = prev_sibling_clone; >> -- >> 2.39.0 >>
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc index f15cb47c8b8..2e7d77ffd6c 100644 --- a/gcc/cgraph.cc +++ b/gcc/cgraph.cc @@ -1893,7 +1893,7 @@ cgraph_node::remove (void) else if (clone_of) { clone_of->clones = next_sibling_clone; - if (!clone_of->analyzed && !clone_of->clones && !clones) + if (!clone_of->analyzed && !clone_of->clones && !clones && !clone_of->clone_of) clone_of->release_body (); } if (next_sibling_clone)