Re: [PATCH] c++: remove optimize_specialization_lookup_p

[Jason Merrill <jason@redhat.com>]

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();
> +}