* [PATCH] c++: remove optimize_specialization_lookup_p
@ 2022-10-06 2:02 Patrick Palka
2022-10-06 4:17 ` Jason Merrill
0 siblings, 1 reply; 2+ messages in thread
From: Patrick Palka @ 2022-10-06 2:02 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, nathan, Patrick Palka
Roughly speaking, optimize_specialization_lookup_p returns true
for a non-template member function of a class template, e.g.
template<class T> struct A { int f(); };
The idea behind the optimization in question seems to be that if we want
to look up the specialization A<T>::f [with T=int], then we can just do
a name lookup for f in A<int> and avoid having to maintain a spec_entry
for it in the decl_specializations table.
However, the benefit of this optimization seems questionable because in
order to do the name lookup we need to look up A<T> [with T=int] in the
type_specializations table, which is just as expensive as looking up an
entry in decl_specializations. And according to my experiments using
stdc++.h, range-v3 and libstdc++ tests, the compiler is slightly (<1%)
_faster_ after disabling this optimization!
Additionally, this optimization means that an explicit specialization
won't get recorded in decl_specializations for such a template, which
is a rather arbitrary invariant violation, and apparently breaks the
below modules testcase.
So since this optimization doesn't seem to give a performance benefit,
complicates the explicit specialization story, and affects modules, this
patch proposes to remove it.
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk? Patch generated with -w to suppress noisy whitespace changes.
gcc/cp/ChangeLog:
* pt.cc (optimize_specialization_lookup_p): Remove.
(retrieve_specialization): Assume the above returns false
and simplify accordingly.
(register_specialization): Likewise.
gcc/testsuite/ChangeLog:
* g++.dg/modules/indirect-3_b.C: Expect that the entity
foo::TPL<0>::frob has is tagged as a specialization and
not just a declaration.
* g++.dg/modules/tpl-spec-8_a.H: New test.
* g++.dg/modules/tpl-spec-8_b.C: New test.
---
gcc/cp/pt.cc | 85 +--------------------
gcc/testsuite/g++.dg/modules/indirect-3_b.C | 2 +-
gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H | 9 +++
gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C | 8 ++
4 files changed, 22 insertions(+), 82 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index bce2a777922..c079bbd4268 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -1170,39 +1170,6 @@ maybe_process_partial_specialization (tree type)
return type;
}
-/* Returns nonzero if we can optimize the retrieval of specializations
- for TMPL, a TEMPLATE_DECL. In particular, for such a template, we
- do not use DECL_TEMPLATE_SPECIALIZATIONS at all. */
-
-static inline bool
-optimize_specialization_lookup_p (tree tmpl)
-{
- return (DECL_FUNCTION_TEMPLATE_P (tmpl)
- && DECL_CLASS_SCOPE_P (tmpl)
- /* DECL_CLASS_SCOPE_P holds of T::f even if T is a template
- parameter. */
- && CLASS_TYPE_P (DECL_CONTEXT (tmpl))
- /* The optimized lookup depends on the fact that the
- template arguments for the member function template apply
- purely to the containing class, which is not true if the
- containing class is an explicit or partial
- specialization. */
- && !CLASSTYPE_TEMPLATE_SPECIALIZATION (DECL_CONTEXT (tmpl))
- && !DECL_MEMBER_TEMPLATE_P (tmpl)
- && !DECL_CONV_FN_P (tmpl)
- /* It is possible to have a template that is not a member
- template and is not a member of a template class:
-
- template <typename T>
- struct S { friend A::f(); };
-
- Here, the friend function is a template, but the context does
- not have template information. The optimized lookup relies
- on having ARGS be the template arguments for both the class
- and the function template. */
- && !DECL_UNIQUE_FRIEND_P (DECL_TEMPLATE_RESULT (tmpl)));
-}
-
/* Make sure ARGS doesn't use any inappropriate typedefs; we should have
gone through coerce_template_parms by now. */
@@ -1276,43 +1243,12 @@ retrieve_specialization (tree tmpl, tree args, hashval_t hash)
if (lambda_fn_in_template_p (tmpl))
return NULL_TREE;
- if (optimize_specialization_lookup_p (tmpl))
- {
- /* The template arguments actually apply to the containing
- class. Find the class specialization with those
- arguments. */
- tree class_template = CLASSTYPE_TI_TEMPLATE (DECL_CONTEXT (tmpl));
- tree class_specialization
- = retrieve_specialization (class_template, args, 0);
- if (!class_specialization)
- return NULL_TREE;
-
- /* Find the instance of TMPL. */
- tree fns = get_class_binding (class_specialization, DECL_NAME (tmpl));
- for (ovl_iterator iter (fns); iter; ++iter)
- {
- tree fn = *iter;
- if (tree ti = get_template_info (fn))
- if (TI_TEMPLATE (ti) == tmpl
- /* using-declarations can bring in a different
- instantiation of tmpl as a member of a different
- instantiation of tmpl's class. We don't want those
- here. */
- && DECL_CONTEXT (fn) == class_specialization)
- return fn;
- }
- return NULL_TREE;
- }
- else
- {
- spec_entry *found;
spec_entry elt;
- spec_hash_table *specializations;
-
elt.tmpl = tmpl;
elt.args = args;
elt.spec = NULL_TREE;
+ spec_hash_table *specializations;
if (DECL_CLASS_TEMPLATE_P (tmpl))
specializations = type_specializations;
else
@@ -1320,10 +1256,8 @@ retrieve_specialization (tree tmpl, tree args, hashval_t hash)
if (hash == 0)
hash = spec_hasher::hash (&elt);
- found = specializations->find_with_hash (&elt, hash);
- if (found)
+ if (spec_entry *found = specializations->find_with_hash (&elt, hash))
return found->spec;
- }
return NULL_TREE;
}
@@ -1567,8 +1501,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
hashval_t hash)
{
tree fn;
- spec_entry **slot = NULL;
- spec_entry elt;
gcc_assert ((TREE_CODE (tmpl) == TEMPLATE_DECL && DECL_P (spec))
|| (TREE_CODE (tmpl) == FIELD_DECL
@@ -1589,12 +1521,7 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
instantiation unless and until it is actually needed. */
return spec;
- if (optimize_specialization_lookup_p (tmpl))
- /* We don't put these specializations in the hash table, but we might
- want to give an error about a mismatch. */
- fn = retrieve_specialization (tmpl, args, 0);
- else
- {
+ spec_entry elt;
elt.tmpl = tmpl;
elt.args = args;
elt.spec = spec;
@@ -1602,12 +1529,11 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
if (hash == 0)
hash = spec_hasher::hash (&elt);
- slot = decl_specializations->find_slot_with_hash (&elt, hash, INSERT);
+ spec_entry **slot = decl_specializations->find_slot_with_hash (&elt, hash, INSERT);
if (*slot)
fn = (*slot)->spec;
else
fn = NULL_TREE;
- }
/* We can sometimes try to re-register a specialization that we've
already got. In particular, regenerate_decl_from_template calls
@@ -1704,8 +1630,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
&& !check_specialization_namespace (tmpl))
DECL_CONTEXT (spec) = DECL_CONTEXT (tmpl);
- if (slot != NULL /* !optimize_specialization_lookup_p (tmpl) */)
- {
spec_entry *entry = ggc_alloc<spec_entry> ();
gcc_assert (tmpl && args && spec);
*entry = elt;
@@ -1723,7 +1647,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
specialization would have used it. */
DECL_TEMPLATE_INSTANTIATIONS (tmpl)
= tree_cons (args, spec, DECL_TEMPLATE_INSTANTIATIONS (tmpl));
- }
return spec;
}
diff --git a/gcc/testsuite/g++.dg/modules/indirect-3_b.C b/gcc/testsuite/g++.dg/modules/indirect-3_b.C
index 5bdfc1d36a8..038b01ecab7 100644
--- a/gcc/testsuite/g++.dg/modules/indirect-3_b.C
+++ b/gcc/testsuite/g++.dg/modules/indirect-3_b.C
@@ -23,7 +23,7 @@ namespace bar
// { dg-final { scan-lang-dump {Lazily binding '::foo@foo:.::TPL'@'foo' section} module } }
// { dg-final { scan-lang-dump {Wrote import:-[0-9]* template_decl:'::foo@foo:.::template TPL@foo:.'@foo} module } }
-// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::TPL<0x0>'\n \[1\]=specialization declaration '::foo@foo:.::TPL<0x0>::TPL<0x0>'\n( \[.\]=[^\n]* '\n)* \[.\]=decl definition '::foo@foo:.::TPL<0x0>::frob<0x0>'\n} module } }
+// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::TPL<0x0>'\n \[1\]=specialization definition '::foo@foo:.::TPL<0x0>::frob<0x0>'\n \[2\]=specialization declaration '::foo@foo:.::TPL<0x0>::TPL<0x0>'} module } }
// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::X@foo:.::frob<0x0>'} module } }
// { dg-final { scan-lang-dump {Writing:-[0-9]*'s type spec merge key \(specialization\) type_decl:'::foo@foo:.::TPL<0x0>'} module } }
diff --git a/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H b/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H
new file mode 100644
index 00000000000..5f85556d7d7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H
@@ -0,0 +1,9 @@
+// { dg-additional-options -fmodule-header }
+// { dg-module-cmi {} }
+
+template<class T>
+struct A {
+ static void f() { T::nonexistent; }
+};
+
+template<> inline void A<int>::f() { }
diff --git a/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C b/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C
new file mode 100644
index 00000000000..f23eb370370
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C
@@ -0,0 +1,8 @@
+// { dg-additional-options -fmodules-ts }
+// { dg-do link }
+
+import "tpl-spec-8_a.H";
+
+int main() {
+ A<int>::f();
+}
--
2.38.0.rc2
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] c++: remove optimize_specialization_lookup_p
2022-10-06 2:02 [PATCH] c++: remove optimize_specialization_lookup_p Patrick Palka
@ 2022-10-06 4:17 ` Jason Merrill
0 siblings, 0 replies; 2+ messages in thread
From: Jason Merrill @ 2022-10-06 4:17 UTC (permalink / raw)
To: Patrick Palka, gcc-patches; +Cc: nathan
On 10/5/22 22:02, Patrick Palka wrote:
> Roughly speaking, optimize_specialization_lookup_p returns true
> for a non-template member function of a class template, e.g.
>
> template<class T> struct A { int f(); };
>
> The idea behind the optimization in question seems to be that if we want
> to look up the specialization A<T>::f [with T=int], then we can just do
> a name lookup for f in A<int> and avoid having to maintain a spec_entry
> for it in the decl_specializations table.
>
> However, the benefit of this optimization seems questionable because in
> order to do the name lookup we need to look up A<T> [with T=int] in the
> type_specializations table, which is just as expensive as looking up an
> entry in decl_specializations. And according to my experiments using
> stdc++.h, range-v3 and libstdc++ tests, the compiler is slightly (<1%)
> _faster_ after disabling this optimization!
Yeah, probably should have done this when we switched to hash tables.
> Additionally, this optimization means that an explicit specialization
> won't get recorded in decl_specializations for such a template, which
> is a rather arbitrary invariant violation, and apparently breaks the
> below modules testcase.
>
> So since this optimization doesn't seem to give a performance benefit,
> complicates the explicit specialization story, and affects modules, this
> patch proposes to remove it.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk? Patch generated with -w to suppress noisy whitespace changes.
OK.
> gcc/cp/ChangeLog:
>
> * pt.cc (optimize_specialization_lookup_p): Remove.
> (retrieve_specialization): Assume the above returns false
> and simplify accordingly.
> (register_specialization): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/indirect-3_b.C: Expect that the entity
> foo::TPL<0>::frob has is tagged as a specialization and
> not just a declaration.
> * g++.dg/modules/tpl-spec-8_a.H: New test.
> * g++.dg/modules/tpl-spec-8_b.C: New test.
> ---
> gcc/cp/pt.cc | 85 +--------------------
> gcc/testsuite/g++.dg/modules/indirect-3_b.C | 2 +-
> gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H | 9 +++
> gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C | 8 ++
> 4 files changed, 22 insertions(+), 82 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C
>
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index bce2a777922..c079bbd4268 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -1170,39 +1170,6 @@ maybe_process_partial_specialization (tree type)
> return type;
> }
>
> -/* Returns nonzero if we can optimize the retrieval of specializations
> - for TMPL, a TEMPLATE_DECL. In particular, for such a template, we
> - do not use DECL_TEMPLATE_SPECIALIZATIONS at all. */
> -
> -static inline bool
> -optimize_specialization_lookup_p (tree tmpl)
> -{
> - return (DECL_FUNCTION_TEMPLATE_P (tmpl)
> - && DECL_CLASS_SCOPE_P (tmpl)
> - /* DECL_CLASS_SCOPE_P holds of T::f even if T is a template
> - parameter. */
> - && CLASS_TYPE_P (DECL_CONTEXT (tmpl))
> - /* The optimized lookup depends on the fact that the
> - template arguments for the member function template apply
> - purely to the containing class, which is not true if the
> - containing class is an explicit or partial
> - specialization. */
> - && !CLASSTYPE_TEMPLATE_SPECIALIZATION (DECL_CONTEXT (tmpl))
> - && !DECL_MEMBER_TEMPLATE_P (tmpl)
> - && !DECL_CONV_FN_P (tmpl)
> - /* It is possible to have a template that is not a member
> - template and is not a member of a template class:
> -
> - template <typename T>
> - struct S { friend A::f(); };
> -
> - Here, the friend function is a template, but the context does
> - not have template information. The optimized lookup relies
> - on having ARGS be the template arguments for both the class
> - and the function template. */
> - && !DECL_UNIQUE_FRIEND_P (DECL_TEMPLATE_RESULT (tmpl)));
> -}
> -
> /* Make sure ARGS doesn't use any inappropriate typedefs; we should have
> gone through coerce_template_parms by now. */
>
> @@ -1276,43 +1243,12 @@ retrieve_specialization (tree tmpl, tree args, hashval_t hash)
> if (lambda_fn_in_template_p (tmpl))
> return NULL_TREE;
>
> - if (optimize_specialization_lookup_p (tmpl))
> - {
> - /* The template arguments actually apply to the containing
> - class. Find the class specialization with those
> - arguments. */
> - tree class_template = CLASSTYPE_TI_TEMPLATE (DECL_CONTEXT (tmpl));
> - tree class_specialization
> - = retrieve_specialization (class_template, args, 0);
> - if (!class_specialization)
> - return NULL_TREE;
> -
> - /* Find the instance of TMPL. */
> - tree fns = get_class_binding (class_specialization, DECL_NAME (tmpl));
> - for (ovl_iterator iter (fns); iter; ++iter)
> - {
> - tree fn = *iter;
> - if (tree ti = get_template_info (fn))
> - if (TI_TEMPLATE (ti) == tmpl
> - /* using-declarations can bring in a different
> - instantiation of tmpl as a member of a different
> - instantiation of tmpl's class. We don't want those
> - here. */
> - && DECL_CONTEXT (fn) == class_specialization)
> - return fn;
> - }
> - return NULL_TREE;
> - }
> - else
> - {
> - spec_entry *found;
> spec_entry elt;
> - spec_hash_table *specializations;
> -
> elt.tmpl = tmpl;
> elt.args = args;
> elt.spec = NULL_TREE;
>
> + spec_hash_table *specializations;
> if (DECL_CLASS_TEMPLATE_P (tmpl))
> specializations = type_specializations;
> else
> @@ -1320,10 +1256,8 @@ retrieve_specialization (tree tmpl, tree args, hashval_t hash)
>
> if (hash == 0)
> hash = spec_hasher::hash (&elt);
> - found = specializations->find_with_hash (&elt, hash);
> - if (found)
> + if (spec_entry *found = specializations->find_with_hash (&elt, hash))
> return found->spec;
> - }
>
> return NULL_TREE;
> }
> @@ -1567,8 +1501,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
> hashval_t hash)
> {
> tree fn;
> - spec_entry **slot = NULL;
> - spec_entry elt;
>
> gcc_assert ((TREE_CODE (tmpl) == TEMPLATE_DECL && DECL_P (spec))
> || (TREE_CODE (tmpl) == FIELD_DECL
> @@ -1589,12 +1521,7 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
> instantiation unless and until it is actually needed. */
> return spec;
>
> - if (optimize_specialization_lookup_p (tmpl))
> - /* We don't put these specializations in the hash table, but we might
> - want to give an error about a mismatch. */
> - fn = retrieve_specialization (tmpl, args, 0);
> - else
> - {
> + spec_entry elt;
> elt.tmpl = tmpl;
> elt.args = args;
> elt.spec = spec;
> @@ -1602,12 +1529,11 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
> if (hash == 0)
> hash = spec_hasher::hash (&elt);
>
> - slot = decl_specializations->find_slot_with_hash (&elt, hash, INSERT);
> + spec_entry **slot = decl_specializations->find_slot_with_hash (&elt, hash, INSERT);
> if (*slot)
> fn = (*slot)->spec;
> else
> fn = NULL_TREE;
> - }
>
> /* We can sometimes try to re-register a specialization that we've
> already got. In particular, regenerate_decl_from_template calls
> @@ -1704,8 +1630,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
> && !check_specialization_namespace (tmpl))
> DECL_CONTEXT (spec) = DECL_CONTEXT (tmpl);
>
> - if (slot != NULL /* !optimize_specialization_lookup_p (tmpl) */)
> - {
> spec_entry *entry = ggc_alloc<spec_entry> ();
> gcc_assert (tmpl && args && spec);
> *entry = elt;
> @@ -1723,7 +1647,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend,
> specialization would have used it. */
> DECL_TEMPLATE_INSTANTIATIONS (tmpl)
> = tree_cons (args, spec, DECL_TEMPLATE_INSTANTIATIONS (tmpl));
> - }
>
> return spec;
> }
> diff --git a/gcc/testsuite/g++.dg/modules/indirect-3_b.C b/gcc/testsuite/g++.dg/modules/indirect-3_b.C
> index 5bdfc1d36a8..038b01ecab7 100644
> --- a/gcc/testsuite/g++.dg/modules/indirect-3_b.C
> +++ b/gcc/testsuite/g++.dg/modules/indirect-3_b.C
> @@ -23,7 +23,7 @@ namespace bar
> // { dg-final { scan-lang-dump {Lazily binding '::foo@foo:.::TPL'@'foo' section} module } }
> // { dg-final { scan-lang-dump {Wrote import:-[0-9]* template_decl:'::foo@foo:.::template TPL@foo:.'@foo} module } }
>
> -// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::TPL<0x0>'\n \[1\]=specialization declaration '::foo@foo:.::TPL<0x0>::TPL<0x0>'\n( \[.\]=[^\n]* '\n)* \[.\]=decl definition '::foo@foo:.::TPL<0x0>::frob<0x0>'\n} module } }
> +// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::TPL<0x0>'\n \[1\]=specialization definition '::foo@foo:.::TPL<0x0>::frob<0x0>'\n \[2\]=specialization declaration '::foo@foo:.::TPL<0x0>::TPL<0x0>'} module } }
>
> // { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::X@foo:.::frob<0x0>'} module } }
> // { dg-final { scan-lang-dump {Writing:-[0-9]*'s type spec merge key \(specialization\) type_decl:'::foo@foo:.::TPL<0x0>'} module } }
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H b/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H
> new file mode 100644
> index 00000000000..5f85556d7d7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H
> @@ -0,0 +1,9 @@
> +// { dg-additional-options -fmodule-header }
> +// { dg-module-cmi {} }
> +
> +template<class T>
> +struct A {
> + static void f() { T::nonexistent; }
> +};
> +
> +template<> inline void A<int>::f() { }
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C b/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C
> new file mode 100644
> index 00000000000..f23eb370370
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C
> @@ -0,0 +1,8 @@
> +// { dg-additional-options -fmodules-ts }
> +// { dg-do link }
> +
> +import "tpl-spec-8_a.H";
> +
> +int main() {
> + A<int>::f();
> +}
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-10-06 4:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 2:02 [PATCH] c++: remove optimize_specialization_lookup_p Patrick Palka
2022-10-06 4:17 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).