From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 16D543858D28 for ; Fri, 24 Mar 2023 12:48:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 16D543858D28 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 D6CF933693; Fri, 24 Mar 2023 12:40:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1679661608; 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: in-reply-to:in-reply-to:references:references; bh=JZp7lHaekYvSJl8UJYhjPFj6cm7eIdvXqsblFe9oCk4=; b=A17thebqJ9asb7/pgyBem5Q+g+vW3bCiasoFW3rO01yhtVL/3cXg1/ZtnGTYIDXGybz9tE YVxi56ifFDeYEgmtmzQ4rr1/0XRCYVX+DMXWF+pczn/h0B1XrYk2vXfvSrAYvm8sb3OV4k bc+NE7W/NgPFv0zXiZnBMlOFZWz9VaU= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1679661608; 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: in-reply-to:in-reply-to:references:references; bh=JZp7lHaekYvSJl8UJYhjPFj6cm7eIdvXqsblFe9oCk4=; b=tlI1jmqdPDwm3Fv3qAjHnvNAwKhHuIqxld6/VbdNUJAdtX6W5l81QOcF/nEUMKbsBgtnZK JI2wN8L9hh7VVEAA== 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 C8B0A138ED; Fri, 24 Mar 2023 12:40:08 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id YaAAMSiaHWRvDgAAMHmgww (envelope-from ); Fri, 24 Mar 2023 12:40:08 +0000 From: Martin Jambor To: Arsen =?utf-8?Q?Arsenovi=C4=87?= Cc: GCC Mailing List , Jan Hubicka , Martin Liska Subject: Re: cgraph: does node->inlined_to imply node->clones is non-empty? In-Reply-To: <86cz55c15x.fsf@aarsen.me> References: <86bkkwude3.fsf@aarsen.me> <86cz55c15x.fsf@aarsen.me> User-Agent: Notmuch/0.37 (https://notmuchmail.org) Emacs/28.2 (x86_64-suse-linux-gnu) Date: Fri, 24 Mar 2023 13:40:08 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi, On Sat, Mar 18 2023, Arsen Arsenovi=C4=87 wrote: > Martin Jambor writes: [...] >>> >>> 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 f= urther >>> 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) If the assert fails, the algorithm does not work as intended. OTOH, It could be a gcc_checking_assert only since user compiled code would still work, just would be unnecessarily bigger. > >>> >>> 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: IPA-inline calls remove_unreachable_nodes to get rid of call graph nodes which are known not to be necessary after inlining (inlining can lead to redirection of some call graph edges to __builtin_unreachable) and unreachable removal... well.. removes nodes. > > (gdb) bt > #0 cgraph_edge::remove_callee ( > this=3D "__ct_base "/18> -> )>) > at ../../gcc/gcc/cgraph.h:3299 > #1 0x0000000000d03c37 in cgraph_node::remove_callees (this=3D * const 0x7ffff6dedaa0 "__ct_base "/18>) at > ../../gcc/gcc/cgraph.cc:1743 > #2 0x0000000000d04387 in cgraph_node::remove (this=3D 0x7ffff6dedaa0 "__ct_base "/18>) 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? Ummm.... the function does that through the call to symtab_node::unregister. But how is that related? > >>> >>> 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? The thing is that ICF discovers that a virtual function, which was originally a possible target, is semantically identically to a non-virtual function and merges them, making the virtual function an alias of the non-virtual one. Devirtualization can then suddenly spit out non-virtual potential targets which is kind of unexpected. It seems to me that the most correct fix is to add to walk_polymorphic_call_targets a check that the obtained possible target is still referenced_from_vtable_p() - because the alias that was originally a virtual function is referenced from a vtable that at this point is also known to be gone. But the check looks like it is possibly expensive, so I wanted to discuss this with Honza first (hopefully next week). > > 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 :) It is good thing that you asked, I also learned something new (that virtual and non-virtual functions can be ICFed together). Martin