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

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

[Martin Liška]

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

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

[Martin Liška]

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)

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

[Martin Liška]

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

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

[Martin Jambor]

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

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

[Jan Hubicka]

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

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

[Martin Liška]

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

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

[Martin Jambor]

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

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

[Jan Hubicka]

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

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

[Martin Jambor]

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