cgraph: does node->inlined_to imply node->clones is non-empty?

[Martin Jambor]

Hello,

I had been aware of your message even before Martin Liška pointed to it,
but I could not answer the questions without looking into the problem
myself.

On Mon, Mar 13 2023, Arsen Arsenović 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!

> 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.

>
> 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.

>
> This decision later trips up the gcc_assert in:
>
>   /* Inline clones might be kept around so their materializing allows further
>      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 = NULL;
> 	  update_inlined_to_pointer (node, node);
> 	}
>       node->aux = 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.

>
> 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).

>
> 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.

> (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 = 0

Or even

  (gdb) p referenced_from_vtable_p(n)
  $24 = false

Time to dig into ipa-devirt.cc, I guess....

Martin