Re: [PATCH] c++, v3: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]
On 4/24/24 15:47, Jakub Jelinek wrote: On Wed, Apr 24, 2024 at 06:39:33PM -0400, Jason Merrill wrote: --- gcc/cp/decl2.cc.jj 2024-04-23 14:49:41.933186265 +0200 +++ gcc/cp/decl2.cc 2024-04-24 15:17:09.043625729 +0200 @@ -3314,7 +3314,16 @@ tentative_decl_linkage (tree decl) to mark the functions at this point. */ if (DECL_DECLARED_INLINE_P (decl) && (!DECL_IMPLICIT_INSTANTIATION (decl) - || DECL_DEFAULTED_FN (decl))) + || DECL_DEFAULTED_FN (decl) + /* For implicit instantiations of cdtors, +if import_export_decl would use comdat linkage, +make sure to use it right away, so that maybe_clone_body +can use aliases. See PR113208. */ + || (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl) + && (flag_implicit_templates + || flag_implicit_inline_templates) + && flag_weak + && TARGET_SUPPORTS_ALIASES))) It seems wrong to set DECL_INTERFACE_KNOWN for cdtors that might get an explicit instantiation later, and likewise for comdat_linkage when !flag_weak; instead of adding this condition to the if, how about adding an else like else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) /* For implicit instantiations of cdtors, if import_export_decl would use comdat linkage, make sure to use it right away, so that maybe_clone_body can use aliases. See PR113208. */ maybe_make_one_only (decl); ? Then can_alias_cdtor would return false, because it ends with: /* 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; Should we change that DECL_INTERFACE_KNOWN (fn) in there to (DECL_INTERFACE_KNOWN (fn) || something) then and what that something should be? HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)? Yes, I think reorganize to ((DECL_INTERFACE_KNOWN (fn) && !DECL_WEAK (fn) && !DECL_ONE_ONLY (fn)) || (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)) Jason
Re: [PATCH] c++, v3: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]
On Wed, Apr 24, 2024 at 06:39:33PM -0400, Jason Merrill wrote: > > --- gcc/cp/decl2.cc.jj 2024-04-23 14:49:41.933186265 +0200 > > +++ gcc/cp/decl2.cc 2024-04-24 15:17:09.043625729 +0200 > > @@ -3314,7 +3314,16 @@ tentative_decl_linkage (tree decl) > > to mark the functions at this point. */ > > if (DECL_DECLARED_INLINE_P (decl) > > && (!DECL_IMPLICIT_INSTANTIATION (decl) > > - || DECL_DEFAULTED_FN (decl))) > > + || DECL_DEFAULTED_FN (decl) > > + /* For implicit instantiations of cdtors, > > +if import_export_decl would use comdat linkage, > > +make sure to use it right away, so that maybe_clone_body > > +can use aliases. See PR113208. */ > > + || (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl) > > + && (flag_implicit_templates > > + || flag_implicit_inline_templates) > > + && flag_weak > > + && TARGET_SUPPORTS_ALIASES))) > > It seems wrong to set DECL_INTERFACE_KNOWN for cdtors that might get an > explicit instantiation later, and likewise for comdat_linkage when > !flag_weak; instead of adding this condition to the if, how about adding an > else like > > > else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) > > /* For implicit instantiations of cdtors, > > if import_export_decl would use comdat linkage, > > make sure to use it right away, so that maybe_clone_body > > can use aliases. See PR113208. */ > > maybe_make_one_only (decl); > > ? Then can_alias_cdtor would return false, because it ends with: /* 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; Should we change that DECL_INTERFACE_KNOWN (fn) in there to (DECL_INTERFACE_KNOWN (fn) || something) then and what that something should be? HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)? Jakub
Re: [PATCH] c++, v3: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]
On 4/24/24 09:16, Jakub Jelinek wrote: On Wed, Apr 24, 2024 at 10:16:05AM +0100, Jonathan Wakely wrote: That fixes the testcases too, but seems to regress +FAIL: libstdc++-abi/abi_check There are explicit instantiation definitions that should instantiate those types: src/c++17/fs_dir.cc:template class std::__shared_ptr; src/c++17/fs_dir.cc:template class std::__shared_ptr; src/c++17/fs_path.cc:template class std::__shared_ptr; So the missing symbols should be present in cow-fs_dir.o and cow-fs_path.o So this boils down to (cvise reduced): namespace __gnu_cxx { enum _Lock_policy { _S_single, _S_mutex, _S_atomic } const __default_lock_policy = _S_atomic; } namespace std { using __gnu_cxx::__default_lock_policy; using __gnu_cxx::_Lock_policy; template struct __shared_ptr { constexpr __shared_ptr() {} }; namespace filesystem { struct _Dir; struct directory_iterator { __shared_ptr<_Dir> _M_dir; }; void end() { directory_iterator(); } } extern template class __shared_ptr; } namespace fs = std::filesystem; template class std::__shared_ptr; at -O2, previously it used to emit _ZNSt12__shared_ptrINSt10filesystem4_DirELN9__gnu_cxx12_Lock_policyE2EEC2Ev _ZNSt12__shared_ptrINSt10filesystem4_DirELN9__gnu_cxx12_Lock_policyE1EEC2Ev but now no longer does with the yesterday posted PR113208 patch. The following updated patch fixes that by calling note_vague_linkage_fn for the cdtor clones from maybe_clone_body if the flags suggest that the maybe-in-charge cdtor had tentative_decl_linkage -> note_vague_linkage_fn called too. And then I've noticed that in some cases the updated comdat group set by maybe_clone_body (*C5* or *D5*) was then overwritten again by maybe_make_one_only. So the patch tweaks cxx_comdat_group, so that when some comdat group has been chosen already it doesn't try to use some different one. Bootstrapped/regtested on x86_64-linux and i686-linux, this one doesn't regress anything unlike the earlier patch. 2024-04-24 Jakub Jelinek PR lto/113208 * decl2.cc (tentative_decl_linkage): Use comdat_linkage also for implicit instantiations of maybe in charge ctors/dtors if -fimplicit-templates or -fimplicit-inline-templates and -fweak and target supports aliases. * optimize.cc (maybe_clone_body): Call note_vague_linkage_fn on clone if fn has DECL_INTERFACE_KNOWN, DECL_NOT_REALLY_EXTERN and DECL_DEFER_OUTPUT flags set. * decl.cc (cxx_comdat_group): For DECL_CLONED_FUNCTION_P functions if SUPPORTS_ONE_ONLY return DECL_COMDAT_GROUP if already set. * g++.dg/abi/comdat2.C: New test. * g++.dg/abi/comdat3.C: New test. * g++.dg/lto/pr113208_0.C: New test. * g++.dg/lto/pr113208_1.C: New file. * g++.dg/lto/pr113208.h: New file. --- gcc/cp/decl2.cc.jj 2024-04-23 14:49:41.933186265 +0200 +++ gcc/cp/decl2.cc 2024-04-24 15:17:09.043625729 +0200 @@ -3314,7 +3314,16 @@ tentative_decl_linkage (tree decl) to mark the functions at this point. */ if (DECL_DECLARED_INLINE_P (decl) && (!DECL_IMPLICIT_INSTANTIATION (decl) - || DECL_DEFAULTED_FN (decl))) + || DECL_DEFAULTED_FN (decl) + /* For implicit instantiations of cdtors, +if import_export_decl would use comdat linkage, +make sure to use it right away, so that maybe_clone_body +can use aliases. See PR113208. */ + || (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl) + && (flag_implicit_templates + || flag_implicit_inline_templates) + && flag_weak + && TARGET_SUPPORTS_ALIASES))) It seems wrong to set DECL_INTERFACE_KNOWN for cdtors that might get an explicit instantiation later, and likewise for comdat_linkage when !flag_weak; instead of adding this condition to the if, how about adding an else like else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)) /* For implicit instantiations of cdtors, if import_export_decl would use comdat linkage, make sure to use it right away, so that maybe_clone_body can use aliases. See PR113208. */ maybe_make_one_only (decl); ? Jason