Message ID | 20150216170330.GI1746@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
Hi, > Hi! > > As discussed in the PR, in 4.9 we used to clone DECL_SECTION_NAME > through using copy_node on the FUNCTION_DECL, and only in selected places > (e.g. when creating artificial_thunk.*, or when creating virtual clones > of DECL_ONE_ONLY functions) we used to explicitly clear DECL_SECTION_NAME. > In 5 the section name is stored in cgraph node instead, and thus not > copied by default, so we instead need to copy it over to restore previous > behavior, otherwise we break the Linux kernel and various other packages. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk? > > 2015-02-16 Jakub Jelinek <jakub@redhat.com> > James Greenhalgh <james.greenhalgh@arm.com> > > PR ipa/64963 > * cgraphclones.c (cgraph_node::create_virtual_clone): Copy > section if not linkonce. Fix up formatting. > (cgraph_node::create_version_clone_with_body): Copy section. > * trans-mem.c (ipa_tm_create_version): Likewise. > > * gcc.dg/ipa/ipa-clone-1.c: New test. Sorry, for taking so long on this. I made similar patch yesterday just did not get around testing it. Imissed the trans-mem case though :) > > --- gcc/cgraphclones.c.jj 2015-01-09 21:59:44.000000000 +0100 > +++ gcc/cgraphclones.c 2015-02-16 14:02:16.564725881 +0100 > @@ -577,7 +577,7 @@ cgraph_node::create_virtual_clone (vec<c > char *name; > > if (!in_lto_p) > - gcc_checking_assert (tree_versionable_function_p (old_decl)); > + gcc_checking_assert (tree_versionable_function_p (old_decl)); > > gcc_assert (local.can_change_signature || !args_to_skip); > > @@ -617,6 +617,8 @@ cgraph_node::create_virtual_clone (vec<c > ABI support for this. */ > set_new_clone_decl_and_node_flags (new_node); > new_node->clone.tree_map = tree_map; > + if (!DECL_ONE_ONLY (old_decl)) Instead of DECL_ONE_ONLY you want to test implicit_section flag. I think resolving unique section with -ffunction-section is also needed. > + new_node->set_section (this->get_section ()); No need for this->... OK with these changes. > > /* Clones of global symbols or symbols with unique names are unique. */ > if ((TREE_PUBLIC (old_decl) > @@ -1009,6 +1011,7 @@ cgraph_node::create_version_clone_with_b > new_version_node->externally_visible = 0; > new_version_node->local.local = 1; > new_version_node->lowered = true; > + new_version_node->set_section (this->get_section ()); > /* Clones of global symbols or symbols with unique names are unique. */ > if ((TREE_PUBLIC (old_decl) > && !DECL_EXTERNAL (old_decl) > --- gcc/trans-mem.c.jj 2015-01-14 09:55:19.000000000 +0100 > +++ gcc/trans-mem.c 2015-02-16 12:58:01.399808815 +0100 > @@ -4967,6 +4967,7 @@ ipa_tm_create_version (struct cgraph_nod > new_node->externally_visible = old_node->externally_visible; > new_node->lowered = true; > new_node->tm_clone = 1; > + new_node->set_section (old_node->get_section ()); > get_cg_data (&old_node, true)->clone = new_node; > > if (old_node->get_availability () >= AVAIL_INTERPOSABLE) > --- gcc/testsuite/gcc.dg/ipa/ipa-clone-1.c.jj 2015-02-16 14:14:39.041625503 +0100 > +++ gcc/testsuite/gcc.dg/ipa/ipa-clone-1.c 2015-02-16 14:15:31.944760949 +0100 > @@ -0,0 +1,20 @@ > +/* PR ipa/64693 */ > +/* { dg-do compile } */ > +/* { dg-require-named-sections "" } */ > +/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp" } */ > + > +static int __attribute__ ((noinline, section ("test_section"))) > +foo (int arg) > +{ > + return 7 * arg; > +} > + > +int > +bar (int arg) > +{ > + return foo (5); > +} > + > +/* { dg-final { scan-assembler "test_section" } } */ > +/* { dg-final { scan-ipa-dump "Creating a specialized node of foo" "cp" } } */ > +/* { dg-final { cleanup-ipa-dump "cp" } } */ > > Jakub
On Mon, Feb 16, 2015 at 07:23:33PM +0100, Jan Hubicka wrote: > > --- gcc/cgraphclones.c.jj 2015-01-09 21:59:44.000000000 +0100 > > +++ gcc/cgraphclones.c 2015-02-16 14:02:16.564725881 +0100 > > @@ -577,7 +577,7 @@ cgraph_node::create_virtual_clone (vec<c > > char *name; > > > > if (!in_lto_p) > > - gcc_checking_assert (tree_versionable_function_p (old_decl)); > > + gcc_checking_assert (tree_versionable_function_p (old_decl)); > > > > gcc_assert (local.can_change_signature || !args_to_skip); > > > > @@ -617,6 +617,8 @@ cgraph_node::create_virtual_clone (vec<c > > ABI support for this. */ > > set_new_clone_decl_and_node_flags (new_node); > > new_node->clone.tree_map = tree_map; > > + if (!DECL_ONE_ONLY (old_decl)) > > Instead of DECL_ONE_ONLY you want to test implicit_section flag. I think > resolving unique section with -ffunction-section is also needed. DECL_ONE_ONLY was the test that 4.9 has been using here: /* Update the properties. Make clone visible only within this translation unit. Make sure that is not weak also. ??? We cannot use COMDAT linkage because there is no ABI support for this. */ if (DECL_ONE_ONLY (old_decl)) DECL_SECTION_NAME (new_node->decl) = NULL; therefore I wanted to match the 4.9 behavior, before we try something different incrementally. > > + new_node->set_section (this->get_section ()); > > No need for this->... Sure, can change that. Jakub
> On Mon, Feb 16, 2015 at 07:23:33PM +0100, Jan Hubicka wrote: > > > --- gcc/cgraphclones.c.jj 2015-01-09 21:59:44.000000000 +0100 > > > +++ gcc/cgraphclones.c 2015-02-16 14:02:16.564725881 +0100 > > > @@ -577,7 +577,7 @@ cgraph_node::create_virtual_clone (vec<c > > > char *name; > > > > > > if (!in_lto_p) > > > - gcc_checking_assert (tree_versionable_function_p (old_decl)); > > > + gcc_checking_assert (tree_versionable_function_p (old_decl)); > > > > > > gcc_assert (local.can_change_signature || !args_to_skip); > > > > > > @@ -617,6 +617,8 @@ cgraph_node::create_virtual_clone (vec<c > > > ABI support for this. */ > > > set_new_clone_decl_and_node_flags (new_node); > > > new_node->clone.tree_map = tree_map; > > > + if (!DECL_ONE_ONLY (old_decl)) > > > > Instead of DECL_ONE_ONLY you want to test implicit_section flag. I think > > resolving unique section with -ffunction-section is also needed. > > DECL_ONE_ONLY was the test that 4.9 has been using here: > > /* Update the properties. > Make clone visible only within this translation unit. Make sure > that is not weak also. > ??? We cannot use COMDAT linkage because there is no > ABI support for this. */ > if (DECL_ONE_ONLY (old_decl)) > DECL_SECTION_NAME (new_node->decl) = NULL; > > therefore I wanted to match the 4.9 behavior, before we try something > different incrementally. OK. implicit_section is trye only for DECL_ONE_ONLY declarations and those that was named by resolve_unique_section via -ffunction-sections, so the change should be safe, but lets first go with immitating 4.9 behaviour. Incremetnally I think we should fix this and also teach inliner to not make code travel in between user named sections. This may become more issue with an LTO kernel builds. Honza > > > > + new_node->set_section (this->get_section ()); > > > > No need for this->... > > Sure, can change that. > > Jakub
--- gcc/cgraphclones.c.jj 2015-01-09 21:59:44.000000000 +0100 +++ gcc/cgraphclones.c 2015-02-16 14:02:16.564725881 +0100 @@ -577,7 +577,7 @@ cgraph_node::create_virtual_clone (vec<c char *name; if (!in_lto_p) - gcc_checking_assert (tree_versionable_function_p (old_decl)); + gcc_checking_assert (tree_versionable_function_p (old_decl)); gcc_assert (local.can_change_signature || !args_to_skip); @@ -617,6 +617,8 @@ cgraph_node::create_virtual_clone (vec<c ABI support for this. */ set_new_clone_decl_and_node_flags (new_node); new_node->clone.tree_map = tree_map; + if (!DECL_ONE_ONLY (old_decl)) + new_node->set_section (this->get_section ()); /* Clones of global symbols or symbols with unique names are unique. */ if ((TREE_PUBLIC (old_decl) @@ -1009,6 +1011,7 @@ cgraph_node::create_version_clone_with_b new_version_node->externally_visible = 0; new_version_node->local.local = 1; new_version_node->lowered = true; + new_version_node->set_section (this->get_section ()); /* Clones of global symbols or symbols with unique names are unique. */ if ((TREE_PUBLIC (old_decl) && !DECL_EXTERNAL (old_decl) --- gcc/trans-mem.c.jj 2015-01-14 09:55:19.000000000 +0100 +++ gcc/trans-mem.c 2015-02-16 12:58:01.399808815 +0100 @@ -4967,6 +4967,7 @@ ipa_tm_create_version (struct cgraph_nod new_node->externally_visible = old_node->externally_visible; new_node->lowered = true; new_node->tm_clone = 1; + new_node->set_section (old_node->get_section ()); get_cg_data (&old_node, true)->clone = new_node; if (old_node->get_availability () >= AVAIL_INTERPOSABLE) --- gcc/testsuite/gcc.dg/ipa/ipa-clone-1.c.jj 2015-02-16 14:14:39.041625503 +0100 +++ gcc/testsuite/gcc.dg/ipa/ipa-clone-1.c 2015-02-16 14:15:31.944760949 +0100 @@ -0,0 +1,20 @@ +/* PR ipa/64693 */ +/* { dg-do compile } */ +/* { dg-require-named-sections "" } */ +/* { dg-options "-O3 -fipa-cp -fipa-cp-clone -fdump-ipa-cp" } */ + +static int __attribute__ ((noinline, section ("test_section"))) +foo (int arg) +{ + return 7 * arg; +} + +int +bar (int arg) +{ + return foo (5); +} + +/* { dg-final { scan-assembler "test_section" } } */ +/* { dg-final { scan-ipa-dump "Creating a specialized node of foo" "cp" } } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */