From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout-p-101.mailbox.org (mout-p-101.mailbox.org [80.241.56.151]) by sourceware.org (Postfix) with ESMTPS id 693E03858D28 for ; Sat, 18 Mar 2023 19:25:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 693E03858D28 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=aarsen.me Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=aarsen.me Received: from smtp102.mailbox.org (smtp102.mailbox.org [IPv6:2001:67c:2050:b231:465::102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-101.mailbox.org (Postfix) with ESMTPS id 4Pf9v15vd3z9sWJ; Sat, 18 Mar 2023 20:25:17 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aarsen.me; s=MBO0001; t=1679167517; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=sonXgN4t9qfPQtzGi7QygYiegsc278zutTefRU8BCbs=; b=wkAo7l22Ffox8dql7Vem08bRRLjnkt7P3tQ1H+MzrbkwIKptAUHe56ADW/4fgeseHi5zOa OYLKptozKyH3TAGLwLsrFkRusMDUHGOebrK7GvbADwXxWKxWOSUiDV95DO1++36StRiZlS R315bNJo8e+ezGGX1FOf5oQq11vNb8415CWmG0IO4SxcPwpAFWQpcVr08sy50Audqbc+hi fGvVL0pfkYMf7WM3aCUFI1t5wmijseMeLGYRjzQRj2EFDZXf75jruhEk+TK28uwAXa0w+I X/6fwCXsC7BsDTlDXPphUKreiBooXutccVCq4mYPwDaxk80luSoUYCZJAAmF2A== References: <86bkkwude3.fsf@aarsen.me> From: Arsen =?utf-8?Q?Arsenovi=C4=87?= To: Martin Jambor Cc: GCC Mailing List , Jan Hubicka , Martin Liska Subject: Re: cgraph: does node->inlined_to imply node->clones is non-empty? Date: Sat, 18 Mar 2023 16:26:15 +0100 In-reply-to: Message-ID: <86cz55c15x.fsf@aarsen.me> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" X-Rspamd-Queue-Id: 4Pf9v15vd3z9sWJ X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DATE_IN_PAST_03_06,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_INFOUSMEBIZ,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP,WEIRD_PORT autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Martin, Thank you for the thorough response, and apologies for replying so late (I'm busy most hours of most workdays nowadays). Martin Jambor writes: > Hello, > > I had been aware of your message even before Martin Li=C5=A1ka pointed to= it, > but I could not answer the questions without looking into the problem > myself. > > On Mon, Mar 13 2023, Arsen Arsenovi=C4=87 via Gcc wrote: >> Hi, >> >> I was debugging PR96059 and ran into a question which does not seem >> obvious from the code. > > Thanks for looking into old bugs, it really is appreciated! My pleasure. >> When the original inline >> happens, ipa-inline-transform.cc:clone_inlined_nodes decides not to make >> a clone, since the function being cloned is a master clone but with no >> non-inline clones. > > The reason is rather that cloning can simply be avoided if you know that > you do not need an offline copy, for anything, be it other normal calls, > calls from outside of the compilation unit, indirect calls, virtual > calls, calls through aliases, thunks... that you do not need the intact > body of the function to create other inline copies, other specialized > clones... and maybe I forgot about something. But this is an efficiency > thing. Right. I was just trying to be specific about which requirement in the if turned out to be false. >> >> For the test case in the PR, in ipa.cc:remove_unreachable_nodes, GCC >> seems to try to remove an unreachable function that was already inlined >> into a different unreachable function. > > No, it fails to remove it. It is still there but should not have been, > that is the problem. Ah - I see. >> >> This decision later trips up the gcc_assert in: >> >> /* Inline clones might be kept around so their materializing allows fu= rther >> cloning. If the function the clone is inlined into is removed, we = need >> to turn it into normal cone. */ >> FOR_EACH_FUNCTION (node) >> { >> if (node->inlined_to >> && !node->callers) >> { >> gcc_assert (node->clones); >> node->inlined_to =3D NULL; >> update_inlined_to_pointer (node, node); >> } >> node->aux =3D NULL; >> } >> >> .. because it is expecting that an inlined function without callers >> (which is necessarily true here as this function is unreachable and so >> was ->remove ()'d earlier) has clones. > > The assert makes sure that if we encounter an inlined-to node without > any caller, that it merely holds as the holder of the function body for > its other specialized (think IPA-CP) or inline clones. If node->clones > is false, there are no such clones and it was a bug to mark the node as > required during the removal of unreachable nodes. I see. That makes sense. So, this assert is placed here by convenience rather than being this invariant being absolutely required for the purpose of the loop? (it is related, so this placement makes sense, I just want to confirm whether it's a "mandatory" invariant) >> >> Either removing the assertion or making clone_inline_nodes clone when >> there are no existing clones 'fixes' (suppresses, but I haven't verified >> whether the results are correct) the problem. >> >> Is this gcc_assert correct in that an inlined function without callers >> necessarily must have clones? > > It is correct. An inlined function without a caller is even a logical > oxymoron and can only exist if it has the purpose described above (and > even then probably only in a fairly special circumstances). Right. I wasn't quite sure whether setting inlined_to would remove that caller, but if I understood right, it should not. What is interesting, though, is that there is an attempt to remove this node during ipa_inline: (gdb) bt #0 cgraph_edge::remove_callee ( this=3D -> )>) at ../../gcc/gcc/cgraph.h:3299 #1 0x0000000000d03c37 in cgraph_node::remove_callees (this=3D) at ../../gcc/gcc/cgraph.cc:1743 #2 0x0000000000d04387 in cgraph_node::remove (this=3D) at ../../gcc/gcc/cgraph.cc:1884 #3 0x00000000010da74f in symbol_table::remove_unreachable_nodes (this=3D0x7ffff6ddb000, file=3D0x7ffff7a814c0 <_IO_2_1_stderr_>) at ../../gcc/gcc/ipa.cc:518 #4 0x0000000002b51e53 in ipa_inline () at ../../gcc/gcc/ipa-inline.cc:2761 #5 0x0000000002b52cf7 in (anonymous namespace)::pass_ipa_inline::execute (this=3D0x3c8d5b0) at ../../gcc/gcc/ipa-inline.cc:3153 (etc) ... I presume that my assumption that cgraph_node::remove () should remove nodes from the cgraph_node::next chain is wrong? >> >> And as a side question, do all clone nodes have a ->clones pointing to >> the same list of all clones, or are they in a tree-ish arrangement, >> where clones of clones end up forming a tree, with the clone_of pointer >> being a pointer to the parent? > > The latter, they form a tree. I see, that makes sense. Thanks. >> (in this instance, the node that trips >> the assert has a nullptr clone_of and clones value, which would AIUI >> imply that it is the original) > > Yes. > >> This train of thinking doesn't end up involving any devirtualization >> code, which the PR was originally reproduced with, but my current theory >> is that devirtualizing here just exposed an edge case that is decently >> well hidden, rather than it playing a crucial role. > > The inlined function is - I believe erroneously - marked as reachable by > walk_polymorphic_call_targets() within the unreachable analysis - so > devirtualizing is somewhat crucial. > > I believe the real question - to which I don't have an answer yet - is > why does possible_polymorphic_call_targets return a method that is not > virtual? > > (gdb) p n->decl->decl_common.virtual_flag > $19 =3D 0 > > Or even > > (gdb) p referenced_from_vtable_p(n) > $24 =3D false > > Time to dig into ipa-devirt.cc, I guess.... > > Martin I saw your response on the PR itself, thanks for handling that in my absence. With the information you've shared I believe I understand that the fix you provided: an inlined call can no longer be a polymorphic call target, and so reaching an inlined function inside walk_polymorphic_call_targets should not be possible? I had already figured that an error could've likely been in reach-ability analysis, but my time ran low, and I had not confirmed anything, or as little as formalized a theory, so I just wrote the original email instead of following this trail of thought fully. Thank you for your guidance! Have a lovely night :) =2D-=20 Arsen Arsenovi=C4=87 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iOYEARYKAI4WIQT+4rPRE/wAoxYtYGFSwpQwHqLEkwUCZBYQG18UgAAAAAAuAChp c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0RkVF MkIzRDExM0ZDMDBBMzE2MkQ2MDYxNTJDMjk0MzAxRUEyQzQ5MxAcYXJzZW5AYWFy c2VuLm1lAAoJEFLClDAeosSTgPYA/0CqhybcpldlMXgDC+Ry5HAj9ZtzbsLoQMCn ADaB+iX4AP9xCbASFyHdDn1OfH9XGEdvPmBUz2VQtFI3a055pvk0Bw== =8jAb -----END PGP SIGNATURE----- --=-=-=--