ipa/124700: Fix rebuild_references for callback functions

  Hi,
this patch deals with the following situation:

template<typename T>
void foo () {
  GOMP_parallel (foo._omp_fn, ...); 
}

Before callback edges were implemented, if foo got put into a COMDAT
group and subsequently virtually cloned (by ipa-cp, for example), then
the OpenMP kernel wouldn't be put into the same COMDAT group, since it
would be referred to by functions both inside and outside said COMDAT
group.

With callback edges, the kernel may be cloned as well.  If both foo and
foo._omp_fn get cloned, foo's clone will be redirected to the cloned
kernel.  This allows us to put foo._omp_fn into the same COMDAT group as
foo via ipa-comdat.  As the kernel is artificial, it becomes
comdat-local.

However, this redirection is not immediate, leading to the situation in
the PR, where the clones are properly redirected in the call graph, but
the corresponding gimple call is not yet updated.  If one calls
rebuild_references during this time on cloned foo, the reference that
gets rebuilt will still point to the old kernel, which is now in a
COMDAT group, thus creating a reference to a comdat-local symbol outside
of its COMDAT group, leading to a checking ICE.  This patch remedies
that by checking for this case and building a reference to the correct
kernel.

Bootstrapped and regtested on x86_64-linux. OK for master?

Thanks,
Josef

	PR 124700

gcc/ChangeLog:

	* cgraph.cc (cgraph_node::is_clone_of): New method, determines
	whether a node is a descendant of another in the clone tree.
	* cgraph.h (struct cgraph_node): Add decl of is_clone_of.
	* cgraphbuild.cc (mark_address): Fix reference creation for
	cloned callback functions.

libgomp/ChangeLog:

	* testsuite/libgomp.c++/pr124700.C: New test.

Signed-off-by: Josef Melcr <josef.melcr@suse.com>
---
 gcc/cgraph.cc                            | 10 ++++
 gcc/cgraph.h                             |  3 ++
 gcc/cgraphbuild.cc                       | 18 ++++++-
 libgomp/testsuite/libgomp.c++/pr124700.C | 60 ++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 libgomp/testsuite/libgomp.c++/pr124700.C
  

Hello,

On Tue, Apr 07 2026, Josef Melcr wrote:
> Hi,
> this patch deals with the following situation:
>
> template<typename T>
> void foo () {
>   GOMP_parallel (foo._omp_fn, ...); 
> }
>
> Before callback edges were implemented, if foo got put into a COMDAT
> group and subsequently virtually cloned (by ipa-cp, for example), then
> the OpenMP kernel wouldn't be put into the same COMDAT group, since it
> would be referred to by functions both inside and outside said COMDAT
> group.
>
> With callback edges, the kernel may be cloned as well.  If both foo and
> foo._omp_fn get cloned, foo's clone will be redirected to the cloned
> kernel.  This allows us to put foo._omp_fn into the same COMDAT group as
> foo via ipa-comdat.  As the kernel is artificial, it becomes
> comdat-local.
>
> However, this redirection is not immediate, leading to the situation in
> the PR, where the clones are properly redirected in the call graph, but
> the corresponding gimple call is not yet updated.  If one calls
> rebuild_references during this time on cloned foo, the reference that
> gets rebuilt will still point to the old kernel, which is now in a
> COMDAT group, thus creating a reference to a comdat-local symbol outside
> of its COMDAT group, leading to a checking ICE.  This patch remedies
> that by checking for this case and building a reference to the correct
> kernel.

I'd like to add that this is an ordering issue, we do call redirection
of the calls from the clone not as a part of node materialization but as
a part of the inliner transformation pass - but we do rebuild references
there.  But before we get to the redirection, the inline materialization
pass is called on the clone of the kernel and finds the COMDAT
reference.  It is expected that until the call redirection the call
statements are wrong, but it is a bit unfortunate that we use them to
build references during that time.  Still, I think the approach in this
patch is what we want to do.

Some comments inline below:

>
> Bootstrapped and regtested on x86_64-linux. OK for master?
>
> Thanks,
> Josef
>
> 	PR 124700
>
> gcc/ChangeLog:
>
> 	* cgraph.cc (cgraph_node::is_clone_of): New method, determines
> 	whether a node is a descendant of another in the clone tree.
> 	* cgraph.h (struct cgraph_node): Add decl of is_clone_of.
> 	* cgraphbuild.cc (mark_address): Fix reference creation for
> 	cloned callback functions.
>
> libgomp/ChangeLog:
>
> 	* testsuite/libgomp.c++/pr124700.C: New test.

Since this is a compile-only testcase that actually does not depend o
the libgomp library, I think it should go to gcc/testsuite/g++.dg/gomp/
directory instead.

>
> Signed-off-by: Josef Melcr <josef.melcr@suse.com>
> ---
>  gcc/cgraph.cc                            | 10 ++++
>  gcc/cgraph.h                             |  3 ++
>  gcc/cgraphbuild.cc                       | 18 ++++++-
>  libgomp/testsuite/libgomp.c++/pr124700.C | 60 ++++++++++++++++++++++++
>  4 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 libgomp/testsuite/libgomp.c++/pr124700.C
>
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index 90635fb5a89..a291bec3af6 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -3626,6 +3626,16 @@ cgraph_node::only_called_directly_p (void)
>  				       NULL, true);
>  }
>  
> +/* Returns TRUE iff THIS is a descendant of N in the clone tree.  */
> +
> +bool
> +cgraph_node::is_clone_of (cgraph_node *n) const
> +{
> +  for (cgraph_node *walker = clone_of; walker; walker = walker->clone_of)
> +    if (walker == n)
> +      return true;
> +  return false;
> +}
>  
>  /* Collect all callers of NODE.  Worker for collect_callers_of_node.  */
>  
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 39209b53957..36d524e28a9 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -1294,6 +1294,9 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
>       it is not used in any other non-standard way.  */
>    bool only_called_directly_p (void);
>  
> +  /* Returns TRUE iff THIS is a descendant of N in the clone tree.  */
> +  bool is_clone_of (cgraph_node *n) const;
> +
>    /* Turn profile to global0.  Walk into inlined functions.  */
>    void make_profile_local ();
>  
> diff --git a/gcc/cgraphbuild.cc b/gcc/cgraphbuild.cc
> index e50adb955cb..8859031d333 100644
> --- a/gcc/cgraphbuild.cc
> +++ b/gcc/cgraphbuild.cc
> @@ -214,9 +214,25 @@ mark_address (gimple *stmt, tree addr, tree, void *data)
>    addr = get_base_address (addr);
>    if (TREE_CODE (addr) == FUNCTION_DECL)
>      {
> +      cgraph_node *caller = (cgraph_node *) data;
>        cgraph_node *node = cgraph_node::get_create (addr);
> +      /* If NODE was cloned and the caller is a callback-dispatching function,
> +	 the gimple call might not be updated yet.  Check whether that's the
> +	 case and if so, replace NODE with the correct callee.  */
> +      if (cgraph_edge *e = caller->get_edge (stmt); e && e->has_callback)

Please make "cgraph_edge *e = caller->get_edge (stmt)" a separate
statement above the if condition.

> +	{
> +	  for (cgraph_edge *cbe = e->first_callback_edge (); cbe;
> +	       cbe = cbe->next_callback_edge ())

GNU coding style requires that the looping test condition is on a
separate line here, even if it is just four characters.

Please wait until Thursday whether Honza has any comments on the
ordering issue but if not, the patch is OK with the minor nits
corrected.

Thanks!

Martin


> +	    {
> +	      if (cbe->callee->is_clone_of (node))
> +		{
> +		  node = cbe->callee;
> +		  break;
> +		}
> +	    }
> +	}
>        node->mark_address_taken ();
> -      ((symtab_node *)data)->create_reference (node, IPA_REF_ADDR, stmt);
> +      caller->create_reference (node, IPA_REF_ADDR, stmt);
>      }
>    else if (addr && VAR_P (addr)
>  	   && (TREE_STATIC (addr) || DECL_EXTERNAL (addr)))
> diff --git a/libgomp/testsuite/libgomp.c++/pr124700.C b/libgomp/testsuite/libgomp.c++/pr124700.C
> new file mode 100644
> index 00000000000..b95754aded2
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.c++/pr124700.C
> @@ -0,0 +1,60 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +
> +int a (int);
> +struct b
> +{
> +  b ();
> +  b (b &);
> +  ~b ();
> +};
> +template <typename c> b d (c);
> +int ab;
> +void ae (int);
> +struct e
> +{
> +  b g;
> +  template <typename c> e operator<< (c h)
> +  {
> +    d (h);
> +    return *this;
> +  }
> +  b f;
> +};
> +template <int = 0> struct i
> +{
> +  long ah;
> +  int j ()
> +  {
> +    e () << "" << aj << 1 << "" << ah << "" << ak << "";
> +    return ah;
> +  }
> +  char aj;
> +  int ak;
> +};
> +i<> k;
> +template <typename>
> +int
> +ao (bool h)
> +{
> +#pragma omp parallel
> +  {
> +    int as;
> +    for (; as;)
> +      {
> +	int au = k.j ();
> +	ae (au);
> +	if (h)
> +	  b ();
> +      }
> +  }
> +  return a (ab);
> +}
> +int l = ao<short> (true);
> +void
> +az ()
> +{
> +  ao<short> (true);
> +  __builtin_unreachable ();
> +}
> +template int ao<short> (bool);
> -- 
> 2.53.0
  

> Hi,
> this patch deals with the following situation:
> 
> template<typename T>
> void foo () {
>   GOMP_parallel (foo._omp_fn, ...); 
> }
> 
> Before callback edges were implemented, if foo got put into a COMDAT
> group and subsequently virtually cloned (by ipa-cp, for example), then
> the OpenMP kernel wouldn't be put into the same COMDAT group, since it
> would be referred to by functions both inside and outside said COMDAT
> group.
> 
> With callback edges, the kernel may be cloned as well.  If both foo and
> foo._omp_fn get cloned, foo's clone will be redirected to the cloned
> kernel.  This allows us to put foo._omp_fn into the same COMDAT group as
> foo via ipa-comdat.  As the kernel is artificial, it becomes
> comdat-local.
> 
> However, this redirection is not immediate, leading to the situation in
> the PR, where the clones are properly redirected in the call graph, but
> the corresponding gimple call is not yet updated.  If one calls
> rebuild_references during this time on cloned foo, the reference that
> gets rebuilt will still point to the old kernel, which is now in a
> COMDAT group, thus creating a reference to a comdat-local symbol outside
> of its COMDAT group, leading to a checking ICE.  This patch remedies
> that by checking for this case and building a reference to the correct
> kernel.
> 
> Bootstrapped and regtested on x86_64-linux. OK for master?
> 
> Thanks,
> Josef
> 
> 	PR 124700
> 
> gcc/ChangeLog:
> 
> 	* cgraph.cc (cgraph_node::is_clone_of): New method, determines
> 	whether a node is a descendant of another in the clone tree.
> 	* cgraph.h (struct cgraph_node): Add decl of is_clone_of.
> 	* cgraphbuild.cc (mark_address): Fix reference creation for
> 	cloned callback functions.
> 
> libgomp/ChangeLog:
> 
> 	* testsuite/libgomp.c++/pr124700.C: New test.

OK,
thanks!
Honza
  

Hi,

I implemented your suggestions and committed the patch as 
r16-8578-g73ade2eefb5633.  Thanks :)


Josef

On 4/7/26 12:28 PM, Martin Jambor wrote:
> Hello,
>
> On Tue, Apr 07 2026, Josef Melcr wrote:
>> Hi,
>> this patch deals with the following situation:
>>
>> template<typename T>
>> void foo () {
>>    GOMP_parallel (foo._omp_fn, ...);
>> }
>>
>> Before callback edges were implemented, if foo got put into a COMDAT
>> group and subsequently virtually cloned (by ipa-cp, for example), then
>> the OpenMP kernel wouldn't be put into the same COMDAT group, since it
>> would be referred to by functions both inside and outside said COMDAT
>> group.
>>
>> With callback edges, the kernel may be cloned as well.  If both foo and
>> foo._omp_fn get cloned, foo's clone will be redirected to the cloned
>> kernel.  This allows us to put foo._omp_fn into the same COMDAT group as
>> foo via ipa-comdat.  As the kernel is artificial, it becomes
>> comdat-local.
>>
>> However, this redirection is not immediate, leading to the situation in
>> the PR, where the clones are properly redirected in the call graph, but
>> the corresponding gimple call is not yet updated.  If one calls
>> rebuild_references during this time on cloned foo, the reference that
>> gets rebuilt will still point to the old kernel, which is now in a
>> COMDAT group, thus creating a reference to a comdat-local symbol outside
>> of its COMDAT group, leading to a checking ICE.  This patch remedies
>> that by checking for this case and building a reference to the correct
>> kernel.
> I'd like to add that this is an ordering issue, we do call redirection
> of the calls from the clone not as a part of node materialization but as
> a part of the inliner transformation pass - but we do rebuild references
> there.  But before we get to the redirection, the inline materialization
> pass is called on the clone of the kernel and finds the COMDAT
> reference.  It is expected that until the call redirection the call
> statements are wrong, but it is a bit unfortunate that we use them to
> build references during that time.  Still, I think the approach in this
> patch is what we want to do.
>
> Some comments inline below:
>
>> Bootstrapped and regtested on x86_64-linux. OK for master?
>>
>> Thanks,
>> Josef
>>
>> 	PR 124700
>>
>> gcc/ChangeLog:
>>
>> 	* cgraph.cc (cgraph_node::is_clone_of): New method, determines
>> 	whether a node is a descendant of another in the clone tree.
>> 	* cgraph.h (struct cgraph_node): Add decl of is_clone_of.
>> 	* cgraphbuild.cc (mark_address): Fix reference creation for
>> 	cloned callback functions.
>>
>> libgomp/ChangeLog:
>>
>> 	* testsuite/libgomp.c++/pr124700.C: New test.
> Since this is a compile-only testcase that actually does not depend o
> the libgomp library, I think it should go to gcc/testsuite/g++.dg/gomp/
> directory instead.
>
>> Signed-off-by: Josef Melcr <josef.melcr@suse.com>
>> ---
>>   gcc/cgraph.cc                            | 10 ++++
>>   gcc/cgraph.h                             |  3 ++
>>   gcc/cgraphbuild.cc                       | 18 ++++++-
>>   libgomp/testsuite/libgomp.c++/pr124700.C | 60 ++++++++++++++++++++++++
>>   4 files changed, 90 insertions(+), 1 deletion(-)
>>   create mode 100644 libgomp/testsuite/libgomp.c++/pr124700.C
>>
>> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
>> index 90635fb5a89..a291bec3af6 100644
>> --- a/gcc/cgraph.cc
>> +++ b/gcc/cgraph.cc
>> @@ -3626,6 +3626,16 @@ cgraph_node::only_called_directly_p (void)
>>   				       NULL, true);
>>   }
>>   
>> +/* Returns TRUE iff THIS is a descendant of N in the clone tree.  */
>> +
>> +bool
>> +cgraph_node::is_clone_of (cgraph_node *n) const
>> +{
>> +  for (cgraph_node *walker = clone_of; walker; walker = walker->clone_of)
>> +    if (walker == n)
>> +      return true;
>> +  return false;
>> +}
>>   
>>   /* Collect all callers of NODE.  Worker for collect_callers_of_node.  */
>>   
>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
>> index 39209b53957..36d524e28a9 100644
>> --- a/gcc/cgraph.h
>> +++ b/gcc/cgraph.h
>> @@ -1294,6 +1294,9 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
>>        it is not used in any other non-standard way.  */
>>     bool only_called_directly_p (void);
>>   
>> +  /* Returns TRUE iff THIS is a descendant of N in the clone tree.  */
>> +  bool is_clone_of (cgraph_node *n) const;
>> +
>>     /* Turn profile to global0.  Walk into inlined functions.  */
>>     void make_profile_local ();
>>   
>> diff --git a/gcc/cgraphbuild.cc b/gcc/cgraphbuild.cc
>> index e50adb955cb..8859031d333 100644
>> --- a/gcc/cgraphbuild.cc
>> +++ b/gcc/cgraphbuild.cc
>> @@ -214,9 +214,25 @@ mark_address (gimple *stmt, tree addr, tree, void *data)
>>     addr = get_base_address (addr);
>>     if (TREE_CODE (addr) == FUNCTION_DECL)
>>       {
>> +      cgraph_node *caller = (cgraph_node *) data;
>>         cgraph_node *node = cgraph_node::get_create (addr);
>> +      /* If NODE was cloned and the caller is a callback-dispatching function,
>> +	 the gimple call might not be updated yet.  Check whether that's the
>> +	 case and if so, replace NODE with the correct callee.  */
>> +      if (cgraph_edge *e = caller->get_edge (stmt); e && e->has_callback)
> Please make "cgraph_edge *e = caller->get_edge (stmt)" a separate
> statement above the if condition.
>
>> +	{
>> +	  for (cgraph_edge *cbe = e->first_callback_edge (); cbe;
>> +	       cbe = cbe->next_callback_edge ())
> GNU coding style requires that the looping test condition is on a
> separate line here, even if it is just four characters.
>
> Please wait until Thursday whether Honza has any comments on the
> ordering issue but if not, the patch is OK with the minor nits
> corrected.
>
> Thanks!
>
> Martin
>
>
>> +	    {
>> +	      if (cbe->callee->is_clone_of (node))
>> +		{
>> +		  node = cbe->callee;
>> +		  break;
>> +		}
>> +	    }
>> +	}
>>         node->mark_address_taken ();
>> -      ((symtab_node *)data)->create_reference (node, IPA_REF_ADDR, stmt);
>> +      caller->create_reference (node, IPA_REF_ADDR, stmt);
>>       }
>>     else if (addr && VAR_P (addr)
>>   	   && (TREE_STATIC (addr) || DECL_EXTERNAL (addr)))
>> diff --git a/libgomp/testsuite/libgomp.c++/pr124700.C b/libgomp/testsuite/libgomp.c++/pr124700.C
>> new file mode 100644
>> index 00000000000..b95754aded2
>> --- /dev/null
>> +++ b/libgomp/testsuite/libgomp.c++/pr124700.C
>> @@ -0,0 +1,60 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O3" } */
>> +
>> +int a (int);
>> +struct b
>> +{
>> +  b ();
>> +  b (b &);
>> +  ~b ();
>> +};
>> +template <typename c> b d (c);
>> +int ab;
>> +void ae (int);
>> +struct e
>> +{
>> +  b g;
>> +  template <typename c> e operator<< (c h)
>> +  {
>> +    d (h);
>> +    return *this;
>> +  }
>> +  b f;
>> +};
>> +template <int = 0> struct i
>> +{
>> +  long ah;
>> +  int j ()
>> +  {
>> +    e () << "" << aj << 1 << "" << ah << "" << ak << "";
>> +    return ah;
>> +  }
>> +  char aj;
>> +  int ak;
>> +};
>> +i<> k;
>> +template <typename>
>> +int
>> +ao (bool h)
>> +{
>> +#pragma omp parallel
>> +  {
>> +    int as;
>> +    for (; as;)
>> +      {
>> +	int au = k.j ();
>> +	ae (au);
>> +	if (h)
>> +	  b ();
>> +      }
>> +  }
>> +  return a (ab);
>> +}
>> +int l = ao<short> (true);
>> +void
>> +az ()
>> +{
>> +  ao<short> (true);
>> +  __builtin_unreachable ();
>> +}
>> +template int ao<short> (bool);
>> -- 
>> 2.53.0