| Message ID | Zikwc9BTXQErR++G@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | c++, v3: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208] | expand |
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<fs::_Dir>; >> src/c++17/fs_dir.cc:template class >> std::__shared_ptr<fs::recursive_directory_iterator::_Dir_stack>; >> src/c++17/fs_path.cc:template class std::__shared_ptr<const >> fs::filesystem_error::_Impl>; >> >> 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 <typename, _Lock_policy = __default_lock_policy> 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<filesystem::_Dir>; > } > namespace fs = std::filesystem; > template class std::__shared_ptr<fs::_Dir>; > 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 <jakub@redhat.com> > > 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
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
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
--- 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))) { /* This function must have external linkage, as otherwise DECL_INTERFACE_KNOWN would have been --- gcc/cp/optimize.cc.jj 2024-04-24 13:44:26.456634100 +0200 +++ gcc/cp/optimize.cc 2024-04-24 14:46:13.050371557 +0200 @@ -717,6 +717,10 @@ maybe_clone_body (tree fn) else expand_or_defer_fn (clone); first = false; + if (DECL_INTERFACE_KNOWN (fn) + && DECL_NOT_REALLY_EXTERN (fn) + && DECL_DEFER_OUTPUT (fn)) + note_vague_linkage_fn (clone); } pop_from_top_level (); --- gcc/cp/decl.cc.jj 2024-04-23 08:31:05.515161033 +0200 +++ gcc/cp/decl.cc 2024-04-24 15:15:34.401951491 +0200 @@ -19254,6 +19254,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/comdat2.C.jj 2024-04-24 13:44:11.054849035 +0200 +++ gcc/testsuite/g++.dg/abi/comdat2.C 2024-04-24 13:44:11.054849035 +0200 @@ -0,0 +1,26 @@ +// PR lto/113208 +// { dg-do compile { target { c++11 && { *-*-*gnu* } } } } +// { dg-additional-options "-O2 -fkeep-inline-functions" } +// { 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); +} --- gcc/testsuite/g++.dg/abi/comdat3.C.jj 2024-04-24 15:23:58.829885362 +0200 +++ gcc/testsuite/g++.dg/abi/comdat3.C 2024-04-24 15:24:46.088223353 +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/lto/pr113208_0.C.jj 2024-04-24 13:43:17.887591040 +0200 +++ gcc/testsuite/g++.dg/lto/pr113208_0.C 2024-04-24 13:44:11.055849021 +0200 @@ -0,0 +1,13 @@ +// { dg-lto-do link } +// { dg-lto-options { {-O1 -std=c++20 -flto}} } +// { dg-extra-ld-options "-r -nostdlib -flinker-output=nolto-rel" } +// { dg-require-linker-plugin "" } + +#define CONSTEXPR constexpr +#include "pr113208.h" + +struct QualityValue; +struct k : vector<QualityValue> {}; + +void m(k); +void n(k i) { m(i); } --- gcc/testsuite/g++.dg/lto/pr113208_1.C.jj 2024-04-24 13:43:17.887591040 +0200 +++ gcc/testsuite/g++.dg/lto/pr113208_1.C 2024-04-24 13:44:11.055849021 +0200 @@ -0,0 +1,6 @@ +#define CONSTEXPR +#include "pr113208.h" + +struct QualityValue; +vector<QualityValue> values1; +vector<QualityValue> values{values1}; --- gcc/testsuite/g++.dg/lto/pr113208.h.jj 2024-04-24 13:43:17.887591040 +0200 +++ gcc/testsuite/g++.dg/lto/pr113208.h 2024-04-24 13:44:11.054849035 +0200 @@ -0,0 +1,10 @@ +template <typename _Tp> struct _Vector_base { + int g() const; + _Vector_base(int, int); +}; +template <typename _Tp> +struct vector : _Vector_base<_Tp> { + CONSTEXPR vector(const vector &__x) + : _Vector_base<_Tp>(1, __x.g()) {} + vector() : _Vector_base<_Tp>(1, 2) {} +};