Re: [PATCH] c++: redundant targ coercion for var/alias tmpls

[Patrick Palka <ppalka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 16580 bytes --]

On Fri, 14 Jul 2023, Jason Merrill wrote:

> On 7/14/23 14:07, Patrick Palka wrote:
> > On Thu, 13 Jul 2023, Jason Merrill wrote:
> > 
> > > On 7/13/23 11:48, Patrick Palka wrote:
> > > > On Wed, 28 Jun 2023, Patrick Palka wrote:
> > > > 
> > > > > On Wed, Jun 28, 2023 at 11:50 AM Jason Merrill <jason@redhat.com>
> > > > > wrote:
> > > > > > 
> > > > > > On 6/23/23 12:23, Patrick Palka wrote:
> > > > > > > On Fri, 23 Jun 2023, Jason Merrill wrote:
> > > > > > > 
> > > > > > > > On 6/21/23 13:19, Patrick Palka wrote:
> > > > > > > > > When stepping through the variable/alias template
> > > > > > > > > specialization
> > > > > > > > > code
> > > > > > > > > paths, I noticed we perform template argument coercion twice:
> > > > > > > > > first from
> > > > > > > > > instantiate_alias_template / finish_template_variable and
> > > > > > > > > again
> > > > > > > > > from
> > > > > > > > > tsubst_decl (during instantiate_template).  It should suffice
> > > > > > > > > to
> > > > > > > > > perform
> > > > > > > > > coercion once.
> > > > > > > > > 
> > > > > > > > > To that end patch elides this second coercion from tsubst_decl
> > > > > > > > > when
> > > > > > > > > possible.  We can't get rid of it completely because we don't
> > > > > > > > > always
> > > > > > > > > specialize a variable template from finish_template_variable:
> > > > > > > > > we
> > > > > > > > > could
> > > > > > > > > also be doing so directly from instantiate_template during
> > > > > > > > > variable
> > > > > > > > > template partial specialization selection, in which case the
> > > > > > > > > coercion
> > > > > > > > > from tsubst_decl would be the first and only coercion.
> > > > > > > > 
> > > > > > > > Perhaps we should be coercing in lookup_template_variable rather
> > > > > > > > than
> > > > > > > > finish_template_variable?
> > > > > > > 
> > > > > > > Ah yes, there's a patch for that at
> > > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/617377.html :)
> > > > > > 
> > > > > > So after that patch, can we get rid of the second coercion
> > > > > > completely?
> > > > > 
> > > > > On second thought it should be possible to get rid of it, if we
> > > > > rearrange things to always pass the primary arguments to tsubst_decl,
> > > > > and perform partial specialization selection from there instead of
> > > > > instantiate_template.  Let me try...
> > > > 
> > > > Like so?  Bootstrapped and regtested on x86_64-pc-linux-gnu.
> > > > 
> > > > -- >8 --
> > > > 
> > > > When stepping through the variable/alias template specialization code
> > > > paths, I noticed we perform template argument coercion twice: first from
> > > > instantiate_alias_template / finish_template_variable and again from
> > > > tsubst_decl (during instantiate_template).  It'd be good to avoid this
> > > > redundant coercion.
> > > > 
> > > > It turns out that this coercion could be safely elided whenever
> > > > specializing a primary variable/alias template, because we can rely on
> > > > lookup_template_variable and instantiate_alias_template to already have
> > > > coerced the arguments.
> > > > 
> > > > The other situation to consider is when fully specializing a partial
> > > > variable template specialization (from instantiate_template), in which
> > > > case the passed 'args' are the (already coerced) arguments relative to
> > > > the partial template and 'argvec', the result of substitution into
> > > > DECL_TI_ARGS, are the (uncoerced) arguments relative to the primary
> > > > template, so coercion is still necessary.  We can still avoid this
> > > > coercion however if we always pass the primary variable template to
> > > > tsubst_decl from instantiate_template, and instead perform partial
> > > > specialization selection directly from tsubst_decl.  This patch
> > > > implements this approach.
> > > 
> > > The relationship between instantiate_template and tsubst_decl is pretty
> > > tangled.  We use the former to substitute (often deduced) template
> > > arguments
> > > into a template, and the latter to substitute template arguments into a
> > > use of
> > > a template...and also to implement the former.
> > > 
> > > For substitution of uses of a template, we expect to need to coerce the
> > > arguments after substitution.  But we avoid this issue for variable
> > > templates
> > > by keeping them as TEMPLATE_ID_EXPR until substitution time, so if we see
> > > a
> > > VAR_DECL in tsubst_decl it's either a non-template variable or under
> > > instantiate_template.
> > 
> > FWIW it seems we could also be in tsubst_decl for a VAR_DECL if
> > 
> >    * we're partially instantiating a class-scope variable template
> >      during instantiation of the class
> 
> Hmm, why don't partial instantiations stay as TEMPLATE_ID_EXPR?

Whoops, I accidentally omitted a crucial word.  The situation is when
partially instantiating a class-scope variable template _declaration_,
e.g. for

  template<class T>
  struct A {
    template<class U> static int v;
  };

  template struct A<int>;

we call tsubst_decl from instantiate_class_template with T=int for the
VAR_DECL for v.

> 
> >    * we're substituting a use of an already non-dependent variable
> >      template specialization
> 
> Sure.
> 
> > > So it seems like the current coercion for variable templates is only
> > > needed in
> > > this case to support the redundant hash table lookup that we just did in
> > > instantiate_template.  Perhaps instead of doing coercion here or moving
> > > the
> > > partial spec lookup, we could skip the hash table lookup for the case of a
> > > variable template?
> > 
> > It seems we'd then also have to make instantiate_template responsible
> > for registering the variable template specialization since tsubst_decl
> > no longer necessarily has the arguments relative to the primary template
> > ('args' could be relative to the partial template).
> > 
> > Like so?  The following makes us perform all the specialization table
> > manipulation in instantiate_template instead of tsubst_decl for variable
> > template specializations.
> 
> Looks good.
> 
> > I wonder if we might want to do this for alias template specializations too?
> 
> That would make sense.

Sounds good, I went ahead and made us do it for function template
specializations too.

> 
> > @@ -15222,20 +15230,21 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
> > complain)
> >   	      {
> >   		tmpl = DECL_TI_TEMPLATE (t);
> >   		gen_tmpl = most_general_template (tmpl);
> > -		argvec = tsubst (DECL_TI_ARGS (t), args, complain, in_decl);
> > -		if (argvec != error_mark_node
> > -		    && PRIMARY_TEMPLATE_P (gen_tmpl)
> > -		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec))
> > -		  /* We're fully specializing a template declaration, so
> > -		     we need to coerce the innermost arguments corresponding
> > to
> > -		     the template.  */
> > -		  argvec = (coerce_template_parms
> > -			    (DECL_TEMPLATE_PARMS (gen_tmpl),
> > -			     argvec, tmpl, complain));
> > -		if (argvec == error_mark_node)
> > -		  RETURN (error_mark_node);
> > -		hash = spec_hasher::hash (gen_tmpl, argvec);
> > -		spec = retrieve_specialization (gen_tmpl, argvec, hash);
> > +		if (variable_template_p (tmpl)
> > +		    && (TMPL_ARGS_DEPTH (args)
> > +			>= TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (gen_tmpl))))
> 
> Do we still need to compare depths?  If not, we could also skip computing
> gen_tmpl in this case.

It was necessary to distinguish the desired instantiate_template case
the "partially specializing a class-scope variable template declaration"
case I clarified above, but not anymore with the new version of the
patch which adds a new parameter to tsubst_decl to control this behavior
instead of inferring it from the other arguments.

How does the following look?  Bootstrapped and regtested on
x86_64-pc-linux-gnu.  Patch formatted with -w to ignore whitespace
changes.

-- >8 --

Subject: [PATCH] c++: redundant targ coercion for var/alias tmpls

gcc/cp/ChangeLog:

	* pt.cc (tsubst_function_decl): Add defaulted 'use_spec_table'
	flag parameter.  Don't look up or insert into the the
	specializations table if 'use_spec_table' is false.
	(tsubst_decl): Add defaulted 'use_spec_table' flag parameter.
	Check for error_mark_node.
	<case FUNCTION_DECL>: Pass 'use_spec_table' to
	tsubst_function_decl.
	<case TYPE/VAR_DECL>: Don't call coerce_template_parms.
	Don't look up or insert into the specializations table if
	'use_spec_table' is false.  Exit earlier if the substituted
	type is erroneous and we're not complaining.
	(instantiate_template): Pass false as 'use_spec_table'
	to tsubst_decl.  Call register_specialization.
---
 gcc/cp/pt.cc | 65 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 255d18b9539..f788127a90f 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -204,7 +204,7 @@ static bool invalid_nontype_parm_type_p (tree, tsubst_flags_t);
 static bool dependent_template_arg_p (tree);
 static bool dependent_type_p_r (tree);
 static tree tsubst_copy	(tree, tree, tsubst_flags_t, tree);
-static tree tsubst_decl (tree, tree, tsubst_flags_t);
+static tree tsubst_decl (tree, tree, tsubst_flags_t, bool = true);
 static tree tsubst_scope (tree, tree, tsubst_flags_t, tree);
 static void perform_instantiation_time_access_checks (tree, tree);
 static tree listify (tree);
@@ -14304,7 +14304,7 @@ maybe_rebuild_function_decl_type (tree decl)
 
 static tree
 tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
-		      tree lambda_fntype)
+		      tree lambda_fntype, bool use_spec_table = true)
 {
   tree gen_tmpl = NULL_TREE, argvec = NULL_TREE;
   hashval_t hash = 0;
@@ -14345,6 +14345,8 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
 
       /* Calculate the complete set of arguments used to
 	 specialize R.  */
+      if (use_spec_table)
+	{
 	  argvec = tsubst_template_args (DECL_TI_ARGS
 					 (DECL_TEMPLATE_RESULT
 					  (DECL_TI_TEMPLATE (t))),
@@ -14362,6 +14364,9 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
 		return STRIP_TEMPLATE (spec);
 	    }
 	}
+      else
+	argvec = args;
+    }
   else
     {
       /* This special case arises when we have something like this:
@@ -14527,12 +14532,15 @@ tsubst_function_decl (tree t, tree args, tsubst_flags_t complain,
 	= build_template_info (gen_tmpl, argvec);
       SET_DECL_IMPLICIT_INSTANTIATION (r);
 
+      if (use_spec_table)
+	{
 	  tree new_r
 	    = register_specialization (r, gen_tmpl, argvec, false, hash);
 	  if (new_r != r)
 	    /* We instantiated this while substituting into
 	       the type earlier (template/friend54.C).  */
 	    return new_r;
+	}
 
       /* We're not supposed to instantiate default arguments
 	 until they are called, for a template.  But, for a
@@ -14855,10 +14863,13 @@ enclosing_instantiation_of (tree tctx)
 
 /* Substitute the ARGS into the T, which is a _DECL.  Return the
    result of the substitution.  Issue error and warning messages under
-   control of COMPLAIN.  */
+   control of COMPLAIN.  The flag USE_SPEC_TABLE controls if we look up
+   and/or register the specialization in the specializations table or
+   if we can assume it's the caller's responsibility.  */
 
 static tree
-tsubst_decl (tree t, tree args, tsubst_flags_t complain)
+tsubst_decl (tree t, tree args, tsubst_flags_t complain,
+	     bool use_spec_table /* = true */)
 {
 #define RETURN(EXP) do { r = (EXP); goto out; } while(0)
   location_t saved_loc;
@@ -14866,6 +14877,9 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
   tree in_decl = t;
   hashval_t hash = 0;
 
+  if (t == error_mark_node)
+    return error_mark_node;
+
   /* Set the filename and linenumber to improve error-reporting.  */
   saved_loc = input_location;
   input_location = DECL_SOURCE_LOCATION (t);
@@ -14879,7 +14893,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
       break;
 
     case FUNCTION_DECL:
-      r = tsubst_function_decl (t, args, complain, /*lambda*/NULL_TREE);
+      r = tsubst_function_decl (t, args, complain, /*lambda*/NULL_TREE,
+				use_spec_table);
       break;
 
     case PARM_DECL:
@@ -15228,22 +15243,18 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	    if (!spec)
 	      {
 		tmpl = DECL_TI_TEMPLATE (t);
-		gen_tmpl = most_general_template (tmpl);
+		if (use_spec_table)
+		  {
 		    argvec = tsubst (DECL_TI_ARGS (t), args, complain, in_decl);
-		if (argvec != error_mark_node
-		    && PRIMARY_TEMPLATE_P (gen_tmpl)
-		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec))
-		  /* We're fully specializing a template declaration, so
-		     we need to coerce the innermost arguments corresponding to
-		     the template.  */
-		  argvec = (coerce_template_parms
-			    (DECL_TEMPLATE_PARMS (gen_tmpl),
-			     argvec, tmpl, complain));
 		    if (argvec == error_mark_node)
 		      RETURN (error_mark_node);
+		    gen_tmpl = most_general_template (tmpl);
 		    hash = spec_hasher::hash (gen_tmpl, argvec);
 		    spec = retrieve_specialization (gen_tmpl, argvec, hash);
 		  }
+		else
+		  argvec = args;
+	      }
 	  }
 	else
 	  {
@@ -15287,20 +15298,20 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	    type = tsubst (type, args, tcomplain, in_decl);
 	    /* Substituting the type might have recursively instantiated this
 	       same alias (c++/86171).  */
-	    if (gen_tmpl && DECL_ALIAS_TEMPLATE_P (gen_tmpl)
+	    if (use_spec_table && gen_tmpl && DECL_ALIAS_TEMPLATE_P (gen_tmpl)
 		&& (spec = retrieve_specialization (gen_tmpl, argvec, hash)))
 	      {
 		r = spec;
 		break;
 	      }
 	  }
+	if (type == error_mark_node && !(complain & tf_error))
+	  RETURN (error_mark_node);
 	r = copy_decl (t);
 	if (VAR_P (r))
 	  {
 	    DECL_INITIALIZED_P (r) = 0;
 	    DECL_TEMPLATE_INSTANTIATED (r) = 0;
-	    if (type == error_mark_node)
-	      RETURN (error_mark_node);
 	    if (TREE_CODE (type) == FUNCTION_TYPE)
 	      {
 		/* It may seem that this case cannot occur, since:
@@ -15404,7 +15415,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 
 	    DECL_TEMPLATE_INFO (r) = build_template_info (tmpl, argvec);
 	    SET_DECL_IMPLICIT_INSTANTIATION (r);
-	    if (!error_operand_p (r) || (complain & tf_error))
+	    if (use_spec_table)
 	      register_specialization (r, gen_tmpl, argvec, false, hash);
 	  }
 	else
@@ -21991,7 +22002,6 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
   tree targ_ptr = orig_args;
   tree fndecl;
   tree gen_tmpl;
-  tree spec;
   bool access_ok = true;
 
   if (tmpl == error_mark_node)
@@ -22038,9 +22048,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
 		(DECL_TI_ARGS (DECL_TEMPLATE_RESULT (tmpl)),
 		 targ_ptr));
 
-  /* It would be nice to avoid hashing here and then again in tsubst_decl,
-     but it doesn't seem to be on the hot path.  */
-  spec = retrieve_specialization (gen_tmpl, targ_ptr, 0);
+  hashval_t hash = spec_hasher::hash (gen_tmpl, targ_ptr);
+  tree spec = retrieve_specialization (gen_tmpl, targ_ptr, hash);
 
   gcc_checking_assert (tmpl == gen_tmpl
 		       || ((fndecl
@@ -22108,13 +22117,14 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
 	  tree partial_tmpl = TI_TEMPLATE (partial_ti);
 	  tree partial_args = TI_ARGS (partial_ti);
 	  tree partial_pat = DECL_TEMPLATE_RESULT (partial_tmpl);
-	  fndecl = tsubst (partial_pat, partial_args, complain, gen_tmpl);
+	  fndecl = tsubst_decl (partial_pat, partial_args, complain,
+				/*use_spec_table=*/false);
 	}
     }
 
   /* Substitute template parameters to obtain the specialization.  */
   if (fndecl == NULL_TREE)
-    fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
+    fndecl = tsubst_decl (pattern, targ_ptr, complain, /*use_spec_table=*/false);
   if (DECL_CLASS_SCOPE_P (gen_tmpl))
     pop_nested_class ();
   pop_from_top_level ();
@@ -22134,6 +22144,11 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
        remember the result of most_specialized_partial_spec for it.  */
     TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
 
+  /* Register the specialization which we prevented tsubst_decl from doing.  */
+  fndecl = register_specialization (fndecl, gen_tmpl, targ_ptr, false, hash);
+  if (fndecl == error_mark_node)
+    return error_mark_node;
+
   set_instantiating_module (fndecl);
 
   /* Now we know the specialization, compute access previously
-- 
2.41.0.337.g830b4a04c4