| Message ID | Zj0T0KbGM9iIZy/+@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Optimize in maybe_clone_body aliases even when not at_eof [PR113208] | expand |
On Thu, May 09, 2024 at 08:20:00PM +0200, Jakub Jelinek wrote: > --- gcc/cp/decl.cc.jj 2024-05-09 10:30:54.804505130 +0200 > +++ gcc/cp/decl.cc 2024-05-09 17:07:08.400110018 +0200 > @@ -19280,6 +19280,14 @@ cxx_comdat_group (tree decl) > else > break; > } > + /* If a ctor/dtor has already set the comdat group by > + maybe_clone_body, don't override it. */ > + if (SUPPORTS_ONE_ONLY > + && TREE_CODE (decl) == FUNCTION_DECL > + && DECL_CLONED_FUNCTION_P (decl) > + && SUPPORTS_ONE_ONLY) > + if (tree comdat = DECL_COMDAT_GROUP (decl)) > + return comdat; This checks SUPPORTS_ONE_ONLY twice. Marek
On Thu, May 09, 2024 at 02:58:52PM -0400, Marek Polacek wrote: > On Thu, May 09, 2024 at 08:20:00PM +0200, Jakub Jelinek wrote: > > --- gcc/cp/decl.cc.jj 2024-05-09 10:30:54.804505130 +0200 > > +++ gcc/cp/decl.cc 2024-05-09 17:07:08.400110018 +0200 > > @@ -19280,6 +19280,14 @@ cxx_comdat_group (tree decl) > > else > > break; > > } > > + /* If a ctor/dtor has already set the comdat group by > > + maybe_clone_body, don't override it. */ > > + if (SUPPORTS_ONE_ONLY > > + && TREE_CODE (decl) == FUNCTION_DECL > > + && DECL_CLONED_FUNCTION_P (decl) > > + && SUPPORTS_ONE_ONLY) > > + if (tree comdat = DECL_COMDAT_GROUP (decl)) > > + return comdat; > > This checks SUPPORTS_ONE_ONLY twice. Oops, you're right, fixed in my copy. Jakub
On 5/9/24 14:20, Jakub Jelinek wrote: > On Thu, Apr 25, 2024 at 11:30:48AM -0400, Jason Merrill wrote: >> Hmm, maybe maybe_clone_body shouldn't clear DECL_SAVED_TREE for aliases, but >> rather set it to some stub like void_node? >> >> Though with all these changes, it's probably better to go with your first >> patch for GCC 14 and delay this approach to 15. Your v1 patch is OK for 14. > > Ok, here is an updated patch, which sets DECL_SAVED_TREE to void_node for > the aliases together with reversion of the earlier committed patch. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2024-05-09 Jakub Jelinek <jakub@redhat.com> > Jason Merrill <jason@redhat.com> > > PR lto/113208 > * cp-tree.h (maybe_optimize_cdtor): Remove. > * decl2.cc (tentative_decl_linkage): Call maybe_make_one_only > for implicit instantiations of maybe in charge ctors/dtors > declared inline. > (import_export_decl): Don't call maybe_optimize_cdtor. > (c_parse_final_cleanups): Formatting fixes. > * optimize.cc (can_alias_cdtor): Adjust condition, for > HAVE_COMDAT_GROUP && DECL_ONE_ONLY && DECL_WEAK return true even > if not DECL_INTERFACE_KNOWN. > --- gcc/cp/optimize.cc.jj 2024-04-25 20:33:30.771858912 +0200 > +++ gcc/cp/optimize.cc 2024-05-09 17:10:23.920478922 +0200 > @@ -220,10 +220,8 @@ can_alias_cdtor (tree fn) > gcc_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn)); > /* Don't use aliases for weak/linkonce definitions unless we can put both > symbols in the same COMDAT group. */ > - return (DECL_INTERFACE_KNOWN (fn) > - && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn)) > - && (!DECL_ONE_ONLY (fn) > - || (HAVE_COMDAT_GROUP && DECL_WEAK (fn)))); > + return (DECL_WEAK (fn) ? (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)) > + : (DECL_INTERFACE_KNOWN (fn) && !DECL_ONE_ONLY (fn))); Hmm, would (DECL_ONE_ONLY (fn) ? HAVE_COMDAT_GROUP : (DECL_INTERFACE_KNOWN (fn) && !DECL_WEAK (fn))) make sense instead? I don't think DECL_WEAK is necessary for COMDAT. Jason
On Fri, May 10, 2024 at 03:59:25PM -0400, Jason Merrill wrote: > > 2024-05-09 Jakub Jelinek <jakub@redhat.com> > > Jason Merrill <jason@redhat.com> > > > > PR lto/113208 > > * cp-tree.h (maybe_optimize_cdtor): Remove. > > * decl2.cc (tentative_decl_linkage): Call maybe_make_one_only > > for implicit instantiations of maybe in charge ctors/dtors > > declared inline. > > (import_export_decl): Don't call maybe_optimize_cdtor. > > (c_parse_final_cleanups): Formatting fixes. > > * optimize.cc (can_alias_cdtor): Adjust condition, for > > HAVE_COMDAT_GROUP && DECL_ONE_ONLY && DECL_WEAK return true even > > if not DECL_INTERFACE_KNOWN. > > > --- gcc/cp/optimize.cc.jj 2024-04-25 20:33:30.771858912 +0200 > > +++ gcc/cp/optimize.cc 2024-05-09 17:10:23.920478922 +0200 > > @@ -220,10 +220,8 @@ can_alias_cdtor (tree fn) > > gcc_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn)); > > /* Don't use aliases for weak/linkonce definitions unless we can put both > > symbols in the same COMDAT group. */ > > - return (DECL_INTERFACE_KNOWN (fn) > > - && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn)) > > - && (!DECL_ONE_ONLY (fn) > > - || (HAVE_COMDAT_GROUP && DECL_WEAK (fn)))); > > + return (DECL_WEAK (fn) ? (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)) > > + : (DECL_INTERFACE_KNOWN (fn) && !DECL_ONE_ONLY (fn))); > > Hmm, would > > (DECL_ONE_ONLY (fn) ? HAVE_COMDAT_GROUP > : (DECL_INTERFACE_KNOWN (fn) && !DECL_WEAK (fn))) > > make sense instead? I don't think DECL_WEAK is necessary for COMDAT. I think it isn't indeed necessary for COMDAT, although e.g. comdat_linkage will not call make_decl_one_only if !flag_weak. But I think it is absolutely required for the alias cdtor optimization in question, because otherwise it would be an ABI change. Consider older version of GCC or some other compiler emitting _ZN6vectorI12QualityValueEC1ERKS1_ and _ZN6vectorI12QualityValueEC2ERKS1_ symbols not as aliases, each in their own comdat groups, so .text._ZN6vectorI12QualityValueEC1ERKS1_ in _ZN6vectorI12QualityValueEC1ERKS1_ comdat group and .text._ZN6vectorI12QualityValueEC2ERKS1_ in _ZN6vectorI12QualityValueEC2ERKS1_ comdat group. And then comes GCC with the above patch without the DECL_WEAK check in there, and decides to use alias, so _ZN6vectorI12QualityValueEC1ERKS1_ is an alias to _ZN6vectorI12QualityValueEC2ERKS1_ and both live in .text._ZN6vectorI12QualityValueEC2ERKS1_ section in _ZN6vectorI12QualityValueEC5ERKS1_ comdat group. If you mix TUs with this, the linker can keep one of the section sets from the _ZN6vectorI12QualityValueEC1ERKS1_ and _ZN6vectorI12QualityValueEC2ERKS1_ and _ZN6vectorI12QualityValueEC5ERKS1_ comdat groups. If there is no .weak for the symbols, this will fail to link, one can emit it either the old way or the new way but never both, it is part of an ABI. While with .weak, mixing it is possible, worst case one gets some unused code in the linked binary or shared library. Of course the desirable case is that there is no mixing and there is no unused code, but if it happens, no big deal. Without .weak it is a big deal. Jakub
On 5/13/24 06:19, Jakub Jelinek wrote: > On Fri, May 10, 2024 at 03:59:25PM -0400, Jason Merrill wrote: >>> 2024-05-09 Jakub Jelinek <jakub@redhat.com> >>> Jason Merrill <jason@redhat.com> >>> >>> PR lto/113208 >>> * cp-tree.h (maybe_optimize_cdtor): Remove. >>> * decl2.cc (tentative_decl_linkage): Call maybe_make_one_only >>> for implicit instantiations of maybe in charge ctors/dtors >>> declared inline. >>> (import_export_decl): Don't call maybe_optimize_cdtor. >>> (c_parse_final_cleanups): Formatting fixes. >>> * optimize.cc (can_alias_cdtor): Adjust condition, for >>> HAVE_COMDAT_GROUP && DECL_ONE_ONLY && DECL_WEAK return true even >>> if not DECL_INTERFACE_KNOWN. >> >>> --- gcc/cp/optimize.cc.jj 2024-04-25 20:33:30.771858912 +0200 >>> +++ gcc/cp/optimize.cc 2024-05-09 17:10:23.920478922 +0200 >>> @@ -220,10 +220,8 @@ can_alias_cdtor (tree fn) >>> gcc_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn)); >>> /* Don't use aliases for weak/linkonce definitions unless we can put both >>> symbols in the same COMDAT group. */ >>> - return (DECL_INTERFACE_KNOWN (fn) >>> - && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn)) >>> - && (!DECL_ONE_ONLY (fn) >>> - || (HAVE_COMDAT_GROUP && DECL_WEAK (fn)))); >>> + return (DECL_WEAK (fn) ? (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)) >>> + : (DECL_INTERFACE_KNOWN (fn) && !DECL_ONE_ONLY (fn))); >> >> Hmm, would >> >> (DECL_ONE_ONLY (fn) ? HAVE_COMDAT_GROUP >> : (DECL_INTERFACE_KNOWN (fn) && !DECL_WEAK (fn))) >> >> make sense instead? I don't think DECL_WEAK is necessary for COMDAT. > > I think it isn't indeed necessary for COMDAT, although e.g. comdat_linkage > will not call make_decl_one_only if !flag_weak. > > But I think it is absolutely required for the alias cdtor optimization > in question, because otherwise it would be an ABI change. > Consider older version of GCC or some other compiler emitting > _ZN6vectorI12QualityValueEC1ERKS1_ > and > _ZN6vectorI12QualityValueEC2ERKS1_ > symbols not as aliases, each in their own comdat groups, so > .text._ZN6vectorI12QualityValueEC1ERKS1_ in _ZN6vectorI12QualityValueEC1ERKS1_ > comdat group and > .text._ZN6vectorI12QualityValueEC2ERKS1_ in _ZN6vectorI12QualityValueEC2ERKS1_ > comdat group. And then comes GCC with the above patch without the DECL_WEAK > check in there, and decides to use alias, so > _ZN6vectorI12QualityValueEC1ERKS1_ is an alias to > _ZN6vectorI12QualityValueEC2ERKS1_ and both live in > .text._ZN6vectorI12QualityValueEC2ERKS1_ section in > _ZN6vectorI12QualityValueEC5ERKS1_ comdat group. If you mix TUs with this, > the linker can keep one of the section sets from the _ZN6vectorI12QualityValueEC1ERKS1_ > and _ZN6vectorI12QualityValueEC2ERKS1_ and _ZN6vectorI12QualityValueEC5ERKS1_ > comdat groups. If there is no .weak for the symbols, this will fail to > link, one can emit it either the old way or the new way but never both, it > is part of an ABI. > While with .weak, mixing it is possible, worst case one gets some unused > code in the linked binary or shared library. Of course the desirable case > is that there is no mixing and there is no unused code, but if it happens, > no big deal. Without .weak it is a big deal. Makes sense, the patch is OK. Jason
--- gcc/cp/cp-tree.h.jj 2024-05-09 10:30:54.775505524 +0200 +++ gcc/cp/cp-tree.h 2024-05-09 17:07:01.246206288 +0200 @@ -7451,7 +7451,6 @@ extern bool handle_module_option (unsign /* In optimize.cc */ extern tree clone_attrs (tree); extern bool maybe_clone_body (tree); -extern void maybe_optimize_cdtor (tree); /* In parser.cc */ extern tree cp_convert_range_for (tree, tree, tree, cp_decomp *, bool, --- gcc/cp/decl2.cc.jj 2024-05-02 09:31:17.753298180 +0200 +++ gcc/cp/decl2.cc 2024-05-09 17:11:11.676836268 +0200 @@ -3325,16 +3325,23 @@ tentative_decl_linkage (tree decl) linkage of all functions, and as that causes writes to the data mapped in from the PCH file, it's advantageous to mark the functions at this point. */ - if (DECL_DECLARED_INLINE_P (decl) - && (!DECL_IMPLICIT_INSTANTIATION (decl) - || DECL_DEFAULTED_FN (decl))) + if (DECL_DECLARED_INLINE_P (decl)) { - /* This function must have external linkage, as - otherwise DECL_INTERFACE_KNOWN would have been - set. */ - gcc_assert (TREE_PUBLIC (decl)); - comdat_linkage (decl); - DECL_INTERFACE_KNOWN (decl) = 1; + if (!DECL_IMPLICIT_INSTANTIATION (decl) + || DECL_DEFAULTED_FN (decl)) + { + /* This function must have external linkage, as + otherwise DECL_INTERFACE_KNOWN would have been + set. */ + gcc_assert (TREE_PUBLIC (decl)); + comdat_linkage (decl); + DECL_INTERFACE_KNOWN (decl) = 1; + } + else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) + /* For implicit instantiations of cdtors try to make + it comdat, so that maybe_clone_body can use aliases. + See PR113208. */ + maybe_make_one_only (decl); } } else if (VAR_P (decl)) @@ -3604,9 +3611,6 @@ import_export_decl (tree decl) } DECL_INTERFACE_KNOWN (decl) = 1; - - if (DECL_CLONED_FUNCTION_P (decl)) - maybe_optimize_cdtor (decl); } /* Return an expression that performs the destruction of DECL, which @@ -5331,7 +5335,7 @@ c_parse_final_cleanups (void) node = node->get_alias_target (); node->call_for_symbol_thunks_and_aliases (clear_decl_external, - NULL, true); + NULL, true); /* If we mark !DECL_EXTERNAL one of the symbols in some comdat group, we need to mark all symbols in the same comdat group that way. */ @@ -5341,7 +5345,7 @@ c_parse_final_cleanups (void) next != node; next = dyn_cast<cgraph_node *> (next->same_comdat_group)) next->call_for_symbol_thunks_and_aliases (clear_decl_external, - NULL, true); + NULL, true); } /* If we're going to need to write this function out, and --- gcc/cp/optimize.cc.jj 2024-04-25 20:33:30.771858912 +0200 +++ gcc/cp/optimize.cc 2024-05-09 17:10:23.920478922 +0200 @@ -220,10 +220,8 @@ can_alias_cdtor (tree fn) gcc_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn)); /* Don't use aliases for weak/linkonce definitions unless we can put both symbols in the same COMDAT group. */ - return (DECL_INTERFACE_KNOWN (fn) - && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn)) - && (!DECL_ONE_ONLY (fn) - || (HAVE_COMDAT_GROUP && DECL_WEAK (fn)))); + return (DECL_WEAK (fn) ? (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)) + : (DECL_INTERFACE_KNOWN (fn) && !DECL_ONE_ONLY (fn))); } /* FN is a [cd]tor, fns is a pointer to an array of length 3. Fill fns @@ -712,7 +710,7 @@ maybe_clone_body (tree fn) if (expand_or_defer_fn_1 (clone)) emit_associated_thunks (clone); /* We didn't generate a body, so remove the empty one. */ - DECL_SAVED_TREE (clone) = NULL_TREE; + DECL_SAVED_TREE (clone) = void_node; } else expand_or_defer_fn (clone); @@ -723,58 +721,3 @@ maybe_clone_body (tree fn) /* We don't need to process the original function any further. */ return 1; } - -/* If maybe_clone_body is called while the cdtor is still tentative, - DECL_ONE_ONLY will be false and so will be can_alias_cdtor (fn). - In that case we wouldn't try to optimize using an alias and instead - would emit separate base and complete cdtor. The following function - attempts to still optimize that case when we import_export_decl - is called first time on one of the clones. */ - -void -maybe_optimize_cdtor (tree orig_decl) -{ - tree fns[3]; - tree fn = DECL_CLONED_FUNCTION (orig_decl); - gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn)); - if (DECL_INTERFACE_KNOWN (fn) - || !TREE_ASM_WRITTEN (fn) - || !DECL_ONE_ONLY (orig_decl) - || symtab->global_info_ready) - return; - - populate_clone_array (fn, fns); - - if (!fns[0] || !fns[1]) - return; - for (int i = 2 - !fns[2]; i >= 0; --i) - if (fns[i] != orig_decl && DECL_INTERFACE_KNOWN (fns[i])) - return; - DECL_INTERFACE_KNOWN (fn) = 1; - comdat_linkage (fn); - if (!can_alias_cdtor (fn)) - return; - /* For comdat base and complete cdtors put them into the same, - *[CD]5* comdat group instead of *[CD][12]*. */ - auto n0 = cgraph_node::get_create (fns[0]); - auto n1 = cgraph_node::get_create (fns[1]); - auto n2 = fns[2] ? cgraph_node::get_create (fns[1]) : NULL; - if (n0->lowered || n1->lowered || (n2 && n2->lowered)) - return; - import_export_decl (fns[0]); - n1->definition = false; - if (!n0->create_same_body_alias (fns[1], fns[0])) - return; - - tree comdat_group = cdtor_comdat_group (fns[1], fns[0]); - n1 = cgraph_node::get (fns[1]); - n0->set_comdat_group (comdat_group); - if (n1->same_comdat_group) - n1->remove_from_same_comdat_group (); - n1->add_to_same_comdat_group (n0); - if (fns[2]) - n2->add_to_same_comdat_group (n0); - import_export_decl (fns[1]); - /* Remove the body now that it is an alias. */ - release_function_body (fns[1]); -} --- gcc/cp/decl.cc.jj 2024-05-09 10:30:54.804505130 +0200 +++ gcc/cp/decl.cc 2024-05-09 17:07:08.400110018 +0200 @@ -19280,6 +19280,14 @@ cxx_comdat_group (tree decl) else break; } + /* If a ctor/dtor has already set the comdat group by + maybe_clone_body, don't override it. */ + if (SUPPORTS_ONE_ONLY + && TREE_CODE (decl) == FUNCTION_DECL + && DECL_CLONED_FUNCTION_P (decl) + && SUPPORTS_ONE_ONLY) + if (tree comdat = DECL_COMDAT_GROUP (decl)) + return comdat; } return decl; --- gcc/testsuite/g++.dg/abi/comdat3.C.jj 2024-05-09 17:07:27.345855068 +0200 +++ gcc/testsuite/g++.dg/abi/comdat3.C 2024-05-09 17:07:08.400110018 +0200 @@ -0,0 +1,22 @@ +// PR lto/113208 +// { dg-do compile { target { c++11 && { *-*-*gnu* } } } } +// { dg-additional-options "-O2" } +// { dg-final { scan-assembler "_ZN1M1SINS_1P1TELN1N1LE2EEC5Ev,comdat" } } +// { dg-final { scan-assembler-not "_ZN1M1SINS_1P1TELN1N1LE2EEC1Ev,comdat" } } +// { dg-final { scan-assembler-not "_ZN1M1SINS_1P1TELN1N1LE2EEC2Ev,comdat" } } + +namespace N { enum L { L1, L2, L3 } const O = L3; } +namespace M { + using N::O; + using N::L; + template <typename, L = O> + struct S { constexpr S () {} }; + namespace P { + struct T; + struct U { S<T> u; }; + void foo () { U (); } + } + extern template class S<P::T>; +} +namespace p = M::P; +template class M::S<p::T>; --- gcc/testsuite/g++.dg/abi/comdat4.C.jj 2024-05-09 17:07:31.961792950 +0200 +++ gcc/testsuite/g++.dg/abi/comdat4.C 2024-05-09 17:07:08.400110018 +0200 @@ -0,0 +1,28 @@ +// PR lto/113208 +// { dg-do compile { target { c++11 && { *-*-*gnu* } } } } +// { dg-additional-options "-O2" } +// { dg-final { scan-assembler "_ZN1BI1CEC5ERKS1_,comdat" } } +// { dg-final { scan-assembler-not "_ZN1BI1CEC1ERKS1_,comdat" } } +// { dg-final { scan-assembler-not "_ZN1BI1CEC2ERKS1_,comdat" } } + +template <typename T> +struct A { + int foo () const; + A (int, int); +}; +template <typename T> +struct B : A<T> { + constexpr B (const B &x) : A<T> (1, x.foo ()) {} + B () : A<T> (1, 2) {} +}; +struct C; +struct D : B<C> {}; +void bar (D); + +void +baz (D x) +{ + bar (x); +} + +template struct B<C>;