Re: [PATCH] IPA: do not release body if still needed

[Martin Jambor <mjambor@suse.cz>]

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