| Message ID | Yl+m6j2J8koAs1Il@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | cgraph: Fix up semantic_interposition handling [PR105306] | expand |
On Wed, 20 Apr 2022, Jakub Jelinek wrote: > Hi! > > cgraph_node has a semantic_interposition flag which should mirror > opt_for_fn (decl, flag_semantic_interposition). But it actually is > initialized not from that, but from flag_semantic_interposition in the > explicit symtab_node (symtab_type t) > : type (t), resolution (LDPR_UNKNOWN), definition (false), alias (false), > ... > semantic_interposition (flag_semantic_interposition), > ... > x_comdat_group (NULL_TREE), x_section (NULL) > {} > ctor. I think that might be fine for varpool nodes, but since > flag_semantic_interposition is now implied from -Ofast it isn't correct > for cgraph nodes, unless we guarantee that cgraph node for a particular > function decl is always created while that function is > current_function_decl. That is often the case, but not always as the > following function shows. > Because symtab_node's ctor doesn't know for which decl the cgraph node > is being created, the following patch keeps that as is, but updates it from > opt_for_fn (decl, flag_semantic_interposition) when we know that, or for > clones copies that flag (often it is then overridden in > set_new_clone_decl_and_node_flags, but not always). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Richard. > 2022-04-20 Jakub Jelinek <jakub@redhat.com> > > PR ipa/105306 > * cgraph.cc (cgraph_node::create): Set node->semantic_interposition > to opt_for_fn (decl, flag_semantic_interposition). > * cgraphclones.cc (cgraph_node::create_clone): Copy over > semantic_interposition flag. > > * g++.dg/opt/pr105306.C: New test. > > --- gcc/cgraph.cc.jj 2022-02-04 14:36:54.069618372 +0100 > +++ gcc/cgraph.cc 2022-04-19 13:38:06.223782974 +0200 > @@ -507,6 +507,7 @@ cgraph_node::create (tree decl) > gcc_assert (TREE_CODE (decl) == FUNCTION_DECL); > > node->decl = decl; > + node->semantic_interposition = opt_for_fn (decl, flag_semantic_interposition); > > if ((flag_openacc || flag_openmp) > && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl))) > --- gcc/cgraphclones.cc.jj 2022-01-18 11:58:58.948991114 +0100 > +++ gcc/cgraphclones.cc 2022-04-19 13:38:43.594262397 +0200 > @@ -394,6 +394,7 @@ cgraph_node::create_clone (tree new_decl > new_node->versionable = versionable; > new_node->can_change_signature = can_change_signature; > new_node->redefined_extern_inline = redefined_extern_inline; > + new_node->semantic_interposition = semantic_interposition; > new_node->tm_may_enter_irr = tm_may_enter_irr; > new_node->externally_visible = false; > new_node->no_reorder = no_reorder; > --- gcc/testsuite/g++.dg/opt/pr105306.C.jj 2022-04-19 13:42:33.908054114 +0200 > +++ gcc/testsuite/g++.dg/opt/pr105306.C 2022-04-19 13:42:08.859403045 +0200 > @@ -0,0 +1,13 @@ > +// PR ipa/105306 > +// { dg-do compile } > +// { dg-options "-Ofast" } > + > +#pragma GCC optimize 0 > +template <typename T> void foo (T); > +struct B { ~B () {} }; > +struct C { B f; }; > +template <typename> struct E { > + void bar () { foo (g); } > + C g; > +}; > +template class E<char>; > > Jakub > >
> On Wed, 20 Apr 2022, Jakub Jelinek wrote: > > > Hi! > > > > cgraph_node has a semantic_interposition flag which should mirror > > opt_for_fn (decl, flag_semantic_interposition). But it actually is > > initialized not from that, but from flag_semantic_interposition in the > > explicit symtab_node (symtab_type t) > > : type (t), resolution (LDPR_UNKNOWN), definition (false), alias (false), > > ... > > semantic_interposition (flag_semantic_interposition), > > ... > > x_comdat_group (NULL_TREE), x_section (NULL) > > {} > > ctor. I think that might be fine for varpool nodes, but since > > flag_semantic_interposition is now implied from -Ofast it isn't correct > > for cgraph nodes, unless we guarantee that cgraph node for a particular > > function decl is always created while that function is > > current_function_decl. That is often the case, but not always as the > > following function shows. Normally cgraph_nodes with function bodies are first created, later finalized and then analyzed. We copy over the semantic_interposition flag from opt_for_fn to cgraph_node in finalize_function. The ctor there is indeed only for varpool nodes since these do not have their opt_for_var. > > --- gcc/cgraph.cc.jj 2022-02-04 14:36:54.069618372 +0100 > > +++ gcc/cgraph.cc 2022-04-19 13:38:06.223782974 +0200 > > @@ -507,6 +507,7 @@ cgraph_node::create (tree decl) > > gcc_assert (TREE_CODE (decl) == FUNCTION_DECL); > > > > node->decl = decl; > > + node->semantic_interposition = opt_for_fn (decl, flag_semantic_interposition); So this change should be unnecessary unless there are nodes that are missing finalization stage. It also is not good enough since frontends may change opt_for_fn between node creation and finalization of compilation unit (so even after cgraph_finalize unforutnately, we had another bug about that). The PR was about implicit C++ alias. So the problem is that aliases bypass finalization becuase they are produced by cgraph_node::create_alias that sets definition flag to true. I guess it would be most consistent to give up on having the flag up to date during cgraph construction (i.e. from finalization time down) and compute it during the cgraph_finalize_complation_unit. I will look into that. > > > > if ((flag_openacc || flag_openmp) > > && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl))) > > --- gcc/cgraphclones.cc.jj 2022-01-18 11:58:58.948991114 +0100 > > +++ gcc/cgraphclones.cc 2022-04-19 13:38:43.594262397 +0200 > > @@ -394,6 +394,7 @@ cgraph_node::create_clone (tree new_decl > > new_node->versionable = versionable; > > new_node->can_change_signature = can_change_signature; > > new_node->redefined_extern_inline = redefined_extern_inline; > > + new_node->semantic_interposition = semantic_interposition; This indeed makes sense to me. Honza > > new_node->tm_may_enter_irr = tm_may_enter_irr; > > new_node->externally_visible = false; > > new_node->no_reorder = no_reorder; > > --- gcc/testsuite/g++.dg/opt/pr105306.C.jj 2022-04-19 13:42:33.908054114 +0200 > > +++ gcc/testsuite/g++.dg/opt/pr105306.C 2022-04-19 13:42:08.859403045 +0200 > > @@ -0,0 +1,13 @@ > > +// PR ipa/105306 > > +// { dg-do compile } > > +// { dg-options "-Ofast" } > > + > > +#pragma GCC optimize 0 > > +template <typename T> void foo (T); > > +struct B { ~B () {} }; > > +struct C { B f; }; > > +template <typename> struct E { > > + void bar () { foo (g); } > > + C g; > > +}; > > +template class E<char>; > > > > Jakub > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, > Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
On Wed, Apr 20, 2022 at 10:45:53AM +0200, Jan Hubicka wrote: > So this change should be unnecessary unless there are nodes that are > missing finalization stage. It also is not good enough since frontends > may change opt_for_fn between node creation and finalization of > compilation unit (so even after cgraph_finalize unforutnately, we had > another bug about that). > > The PR was about implicit C++ alias. So the problem is that aliases > bypass finalization becuase they are produced by > cgraph_node::create_alias that sets definition flag to true. Note, I've already committed the patch as Richi acked it. So, can we move that node->semantic_interposition = opt_for_fn (decl, flag_semantic_interposition); from cgraph_node::create to cgraph_node::create_alias? > I guess it would be most consistent to give up on having the flag up to > date during cgraph construction (i.e. from finalization time down) and > compute it during the cgraph_finalize_complation_unit. I will look into > that. Jakub
> On Wed, Apr 20, 2022 at 10:45:53AM +0200, Jan Hubicka wrote: > > So this change should be unnecessary unless there are nodes that are > > missing finalization stage. It also is not good enough since frontends > > may change opt_for_fn between node creation and finalization of > > compilation unit (so even after cgraph_finalize unforutnately, we had > > another bug about that). > > > > The PR was about implicit C++ alias. So the problem is that aliases > > bypass finalization becuase they are produced by > > cgraph_node::create_alias that sets definition flag to true. > > Note, I've already committed the patch as Richi acked it. > So, can we move that > node->semantic_interposition = opt_for_fn (decl, flag_semantic_interposition); > from cgraph_node::create to cgraph_node::create_alias? I think it would be easiest to move it to the visibility pass (after all it is about visibilities and all earlier uses of the flag are wrong since frontend is changing it at any time until unit is fully built). I will prepare patch tonight or tomorrow. Also thinking about the copying in cgraph_clone, it would make snese only if we produce clones with public linkage. Do we ever do that? Honza > > > I guess it would be most consistent to give up on having the flag up to > > date during cgraph construction (i.e. from finalization time down) and > > compute it during the cgraph_finalize_complation_unit. I will look into > > that. > > Jakub >
On Wed, Apr 20, 2022 at 11:06:12AM +0200, Jan Hubicka wrote: > > On Wed, Apr 20, 2022 at 10:45:53AM +0200, Jan Hubicka wrote: > > > So this change should be unnecessary unless there are nodes that are > > > missing finalization stage. It also is not good enough since frontends > > > may change opt_for_fn between node creation and finalization of > > > compilation unit (so even after cgraph_finalize unforutnately, we had > > > another bug about that). > > > > > > The PR was about implicit C++ alias. So the problem is that aliases > > > bypass finalization becuase they are produced by > > > cgraph_node::create_alias that sets definition flag to true. > > > > Note, I've already committed the patch as Richi acked it. > > So, can we move that > > node->semantic_interposition = opt_for_fn (decl, flag_semantic_interposition); > > from cgraph_node::create to cgraph_node::create_alias? > > I think it would be easiest to move it to the visibility pass > (after all it is about visibilities and all earlier uses of the flag > are wrong since frontend is changing it at any time until unit is fully > built). I will prepare patch tonight or tomorrow. > > Also thinking about the copying in cgraph_clone, it would make snese > only if we produce clones with public linkage. Do we ever do that? The cgraph.cc change was what I actually needed for the fix, the cgraphclones.cc was only because I've noticed that it constructs a new node (so is initialized to whatever random flag_semantic_interposition is right now) and initializing it to what it is cloned from made more sense. Jakub
> > The cgraph.cc change was what I actually needed for the fix, the > cgraphclones.cc was only because I've noticed that it constructs a new > node (so is initialized to whatever random flag_semantic_interposition is > right now) and initializing it to what it is cloned from made more sense. OK, thanks. It only is needed for nodes which definition flag and public linkage, so should not need to copy in cgraph clones and there are other places that creates new nodes (late function etc). I will move the logic to visibility pass and to add_new_function and also kill the constructor. I originally intended to set it at the consturction time but forget to think of the frotnends changing opt_for_fn later from the optimization attribute. This also makes me wonder if C++ FE updates the implicit aliases once they have been created... Honza > > Jakub >
Hi, On Wed, Apr 20 2022, Jan Hubicka via Gcc-patches wrote: >> On Wed, 20 Apr 2022, Jakub Jelinek wrote: [...] >> > >> > if ((flag_openacc || flag_openmp) >> > && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl))) >> > --- gcc/cgraphclones.cc.jj 2022-01-18 11:58:58.948991114 +0100 >> > +++ gcc/cgraphclones.cc 2022-04-19 13:38:43.594262397 +0200 >> > @@ -394,6 +394,7 @@ cgraph_node::create_clone (tree new_decl >> > new_node->versionable = versionable; >> > new_node->can_change_signature = can_change_signature; >> > new_node->redefined_extern_inline = redefined_extern_inline; >> > + new_node->semantic_interposition = semantic_interposition; > > This indeed makes sense to me. but that means theat create_clone (and therefore also create_virtual_clone) now creates nodes which are both local and potentially interposable... is that what we want? (Does the local flag make the interposition flag meaningless in that case?) Martin
On Wed, Apr 20, 2022 at 01:47:43PM +0200, Martin Jambor wrote: > Hi, > > On Wed, Apr 20 2022, Jan Hubicka via Gcc-patches wrote: > >> On Wed, 20 Apr 2022, Jakub Jelinek wrote: > > [...] > > >> > > >> > if ((flag_openacc || flag_openmp) > >> > && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl))) > >> > --- gcc/cgraphclones.cc.jj 2022-01-18 11:58:58.948991114 +0100 > >> > +++ gcc/cgraphclones.cc 2022-04-19 13:38:43.594262397 +0200 > >> > @@ -394,6 +394,7 @@ cgraph_node::create_clone (tree new_decl > >> > new_node->versionable = versionable; > >> > new_node->can_change_signature = can_change_signature; > >> > new_node->redefined_extern_inline = redefined_extern_inline; > >> > + new_node->semantic_interposition = semantic_interposition; > > > > This indeed makes sense to me. > > but that means theat create_clone (and therefore also > create_virtual_clone) now creates nodes which are both local and > potentially interposable... is that what we want? (Does the local flag > make the interposition flag meaningless in that case?) Usually set_new_clone_decl_and_node_flags is called afterwards and that makes both the decl local and clears node->semantic_interposition. The above is just for the case when that isn't done. Jakub
> On Wed, Apr 20, 2022 at 01:47:43PM +0200, Martin Jambor wrote: > > Hi, > > > > On Wed, Apr 20 2022, Jan Hubicka via Gcc-patches wrote: > > >> On Wed, 20 Apr 2022, Jakub Jelinek wrote: > > > > [...] > > > > >> > > > >> > if ((flag_openacc || flag_openmp) > > >> > && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl))) > > >> > --- gcc/cgraphclones.cc.jj 2022-01-18 11:58:58.948991114 +0100 > > >> > +++ gcc/cgraphclones.cc 2022-04-19 13:38:43.594262397 +0200 > > >> > @@ -394,6 +394,7 @@ cgraph_node::create_clone (tree new_decl > > >> > new_node->versionable = versionable; > > >> > new_node->can_change_signature = can_change_signature; > > >> > new_node->redefined_extern_inline = redefined_extern_inline; > > >> > + new_node->semantic_interposition = semantic_interposition; > > > > > > This indeed makes sense to me. > > > > but that means theat create_clone (and therefore also > > create_virtual_clone) now creates nodes which are both local and > > potentially interposable... is that what we want? (Does the local flag > > make the interposition flag meaningless in that case?) > > Usually set_new_clone_decl_and_node_flags is called afterwards and that > makes both the decl local and clears node->semantic_interposition. > The above is just for the case when that isn't done. We also simply ignore semantic_interposition flag on everything local. But indeed perhaps for consistency purposes we should force it to false whenever externally_visible is false. But more sanity checkers only in stage1 :) Honza > > Jakub >
--- gcc/cgraph.cc.jj 2022-02-04 14:36:54.069618372 +0100 +++ gcc/cgraph.cc 2022-04-19 13:38:06.223782974 +0200 @@ -507,6 +507,7 @@ cgraph_node::create (tree decl) gcc_assert (TREE_CODE (decl) == FUNCTION_DECL); node->decl = decl; + node->semantic_interposition = opt_for_fn (decl, flag_semantic_interposition); if ((flag_openacc || flag_openmp) && lookup_attribute ("omp declare target", DECL_ATTRIBUTES (decl))) --- gcc/cgraphclones.cc.jj 2022-01-18 11:58:58.948991114 +0100 +++ gcc/cgraphclones.cc 2022-04-19 13:38:43.594262397 +0200 @@ -394,6 +394,7 @@ cgraph_node::create_clone (tree new_decl new_node->versionable = versionable; new_node->can_change_signature = can_change_signature; new_node->redefined_extern_inline = redefined_extern_inline; + new_node->semantic_interposition = semantic_interposition; new_node->tm_may_enter_irr = tm_may_enter_irr; new_node->externally_visible = false; new_node->no_reorder = no_reorder; --- gcc/testsuite/g++.dg/opt/pr105306.C.jj 2022-04-19 13:42:33.908054114 +0200 +++ gcc/testsuite/g++.dg/opt/pr105306.C 2022-04-19 13:42:08.859403045 +0200 @@ -0,0 +1,13 @@ +// PR ipa/105306 +// { dg-do compile } +// { dg-options "-Ofast" } + +#pragma GCC optimize 0 +template <typename T> void foo (T); +struct B { ~B () {} }; +struct C { B f; }; +template <typename> struct E { + void bar () { foo (g); } + C g; +}; +template class E<char>;
Hi! cgraph_node has a semantic_interposition flag which should mirror opt_for_fn (decl, flag_semantic_interposition). But it actually is initialized not from that, but from flag_semantic_interposition in the explicit symtab_node (symtab_type t) : type (t), resolution (LDPR_UNKNOWN), definition (false), alias (false), ... semantic_interposition (flag_semantic_interposition), ... x_comdat_group (NULL_TREE), x_section (NULL) {} ctor. I think that might be fine for varpool nodes, but since flag_semantic_interposition is now implied from -Ofast it isn't correct for cgraph nodes, unless we guarantee that cgraph node for a particular function decl is always created while that function is current_function_decl. That is often the case, but not always as the following function shows. Because symtab_node's ctor doesn't know for which decl the cgraph node is being created, the following patch keeps that as is, but updates it from opt_for_fn (decl, flag_semantic_interposition) when we know that, or for clones copies that flag (often it is then overridden in set_new_clone_decl_and_node_flags, but not always). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2022-04-20 Jakub Jelinek <jakub@redhat.com> PR ipa/105306 * cgraph.cc (cgraph_node::create): Set node->semantic_interposition to opt_for_fn (decl, flag_semantic_interposition). * cgraphclones.cc (cgraph_node::create_clone): Copy over semantic_interposition flag. * g++.dg/opt/pr105306.C: New test. Jakub