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

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

[Patrick Palka]

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.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?  This reduces memory usage of range-v3's zip.cpp by ~0.5%.

gcc/cp/ChangeLog:

	* pt.cc (tsubst_decl) <case TYPE_/VAR_DECL>: Call
	coercion_template_parms only if DECL_TEMPLATE_SPECIALIZATION
	is set.
---
 gcc/cp/pt.cc | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index be86051abad..dd10409ce18 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -15232,10 +15232,17 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 		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.  */
+		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec)
+		    && DECL_TEMPLATE_SPECIALIZATION (t))
+		  /* We're fully specializing an alias or variable template, so
+		     coerce the innermost arguments if necessary.  We expect
+		     instantiate_alias_template and finish_template_variable to
+		     already have done this relative to the primary template, in
+		     which case this coercion is unnecessary, but we can also
+		     get here when substituting a partial variable template
+		     specialization (directly from instantiate_template), in
+		     which case DECL_TEMPLATE_SPECIALIZATION is set and coercion
+		     is necessary.  */
 		  argvec = (coerce_template_parms
 			    (DECL_TEMPLATE_PARMS (gen_tmpl),
 			     argvec, tmpl, complain));
-- 
2.41.0.113.g6640c2d06d

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

[Jason Merrill]

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?  It looks like we currently get to 
most_specialized_partial_spec with args that haven't yet been coerced to 
match the primary template.

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?  This reduces memory usage of range-v3's zip.cpp by ~0.5%.
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (tsubst_decl) <case TYPE_/VAR_DECL>: Call
> 	coercion_template_parms only if DECL_TEMPLATE_SPECIALIZATION
> 	is set.
> ---
>   gcc/cp/pt.cc | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index be86051abad..dd10409ce18 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -15232,10 +15232,17 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
>   		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.  */
> +		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec)
> +		    && DECL_TEMPLATE_SPECIALIZATION (t))
> +		  /* We're fully specializing an alias or variable template, so
> +		     coerce the innermost arguments if necessary.  We expect
> +		     instantiate_alias_template and finish_template_variable to
> +		     already have done this relative to the primary template, in
> +		     which case this coercion is unnecessary, but we can also
> +		     get here when substituting a partial variable template
> +		     specialization (directly from instantiate_template), in
> +		     which case DECL_TEMPLATE_SPECIALIZATION is set and coercion
> +		     is necessary.  */
>   		  argvec = (coerce_template_parms
>   			    (DECL_TEMPLATE_PARMS (gen_tmpl),
>   			     argvec, tmpl, complain));

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

[Patrick Palka]

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 :)

> It looks like we currently get to
> most_specialized_partial_spec with args that haven't yet been coerced to match
> the primary template.

The call to most_specialized_partial_spec from instantiate_template?
I believe the arguments should already have been coerced by the
caller, which is presumably always finish_template_variable.

So in that patch I also made instantiate_template use
build2 (TEMPLATE_ID_EXPR, ...) directly instead of calling
lookup_template_variable, to avoid an unnecessary double coercion.

> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?  This reduces memory usage of range-v3's zip.cpp by ~0.5%.
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.cc (tsubst_decl) <case TYPE_/VAR_DECL>: Call
> > 	coercion_template_parms only if DECL_TEMPLATE_SPECIALIZATION
> > 	is set.
> > ---
> >   gcc/cp/pt.cc | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index be86051abad..dd10409ce18 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -15232,10 +15232,17 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
> > complain)
> >   		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.  */
> > +		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec)
> > +		    && DECL_TEMPLATE_SPECIALIZATION (t))
> > +		  /* We're fully specializing an alias or variable template,
> > so
> > +		     coerce the innermost arguments if necessary.  We expect
> > +		     instantiate_alias_template and finish_template_variable
> > to
> > +		     already have done this relative to the primary template,
> > in
> > +		     which case this coercion is unnecessary, but we can also
> > +		     get here when substituting a partial variable template
> > +		     specialization (directly from instantiate_template), in
> > +		     which case DECL_TEMPLATE_SPECIALIZATION is set and
> > coercion
> > +		     is necessary.  */
> >   		  argvec = (coerce_template_parms
> >   			    (DECL_TEMPLATE_PARMS (gen_tmpl),
> >   			     argvec, tmpl, complain));
> 
> 

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

[Jason Merrill]

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?

Jason

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

[Patrick Palka]

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...

>
> Jason
>

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

[Patrick Palka]

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

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.

gcc/cp/ChangeLog:

	* pt.cc (tsubst_decl) <case VAR/TYPE_DECL>: Don't call
	coerce_template_parms.  Call most_specialized_partial_spec
	when fully specializing a variable template here ...
	(instantiate_template): ... instead of here.  Always pass
	the primary variable template pattern to tsubst_decl.
---
 gcc/cp/pt.cc | 62 +++++++++++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 35 deletions(-)

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index fa15b75b9c5..53968b823d5 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -15194,6 +15194,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	/* Check to see if we already have the specialization we
 	   need.  */
 	tree spec = NULL_TREE;
+	tree partial_ti = NULL_TREE;
 	bool local_p = false;
 	tree ctx = DECL_CONTEXT (t);
 	if (!(VAR_P (t) && DECL_LOCAL_DECL_P (t))
@@ -15230,17 +15231,29 @@ 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);
+		if (variable_template_p (gen_tmpl)
+		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec))
+		  {
+		    /* We need to determine if we're using a partial
+		       specialization now, because the type of the
+		       variable could be different.  */
+		    iloc_sentinel ils (saved_loc);
+		    tree tid = build_nt (TEMPLATE_ID_EXPR, gen_tmpl, argvec);
+		    partial_ti = most_specialized_partial_spec (tid, complain);
+		    if (partial_ti == error_mark_node)
+		      RETURN (error_mark_node);
+		    else if (partial_ti)
+		      {
+			tree partial_tmpl = TI_TEMPLATE (partial_ti);
+			tree partial_args = TI_ARGS (partial_ti);
+			tree partial_pat = DECL_TEMPLATE_RESULT (partial_tmpl);
+			t = partial_pat;
+			args = partial_args;
+			in_decl = partial_pat;
+		      }
+		  }
 		hash = spec_hasher::hash (gen_tmpl, argvec);
 		spec = retrieve_specialization (gen_tmpl, argvec, hash);
 	      }
@@ -15403,6 +15416,10 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	      DECL_NOT_REALLY_EXTERN (r) = 1;
 
 	    DECL_TEMPLATE_INFO (r) = build_template_info (tmpl, argvec);
+	    if (variable_template_p (tmpl))
+	      /* Now that we we've formed this variable template specialization,
+		 remember the result of most_specialized_partial_spec for it.  */
+	      TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (r)) = partial_ti;
 	    SET_DECL_IMPLICIT_INSTANTIATION (r);
 	    if (!error_operand_p (r) || (complain & tf_error))
 	      register_specialization (r, gen_tmpl, argvec, false, hash);
@@ -22092,29 +22109,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
 
   tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
 
-  tree partial_ti = NULL_TREE;
-  fndecl = NULL_TREE;
-  if (VAR_P (pattern))
-    {
-      /* We need to determine if we're using a partial or explicit
-	 specialization now, because the type of the variable could be
-	 different.  */
-      tree tid = build2 (TEMPLATE_ID_EXPR, NULL_TREE, tmpl, targ_ptr);
-      partial_ti = most_specialized_partial_spec (tid, complain);
-      if (partial_ti == error_mark_node)
-	pattern = error_mark_node;
-      else if (partial_ti)
-	{
-	  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);
-	}
-    }
-
   /* Substitute template parameters to obtain the specialization.  */
-  if (fndecl == NULL_TREE)
-    fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
+  fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
   if (DECL_CLASS_SCOPE_P (gen_tmpl))
     pop_nested_class ();
   pop_from_top_level ();
@@ -22129,10 +22125,6 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
      template, not the most general template.  */
   DECL_TI_TEMPLATE (fndecl) = tmpl;
   DECL_TI_ARGS (fndecl) = targ_ptr;
-  if (VAR_P (pattern))
-    /* Now that we we've formed this variable template specialization,
-       remember the result of most_specialized_partial_spec for it.  */
-    TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
 
   set_instantiating_module (fndecl);
 
-- 
2.41.0.327.gaa9166bcc0

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

[Jason Merrill]

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.

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?

> gcc/cp/ChangeLog:
> 
> 	* pt.cc (tsubst_decl) <case VAR/TYPE_DECL>: Don't call
> 	coerce_template_parms.  Call most_specialized_partial_spec
> 	when fully specializing a variable template here ...
> 	(instantiate_template): ... instead of here.  Always pass
> 	the primary variable template pattern to tsubst_decl.
> ---
>   gcc/cp/pt.cc | 62 +++++++++++++++++++++++-----------------------------
>   1 file changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index fa15b75b9c5..53968b823d5 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -15194,6 +15194,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
>   	/* Check to see if we already have the specialization we
>   	   need.  */
>   	tree spec = NULL_TREE;
> +	tree partial_ti = NULL_TREE;
>   	bool local_p = false;
>   	tree ctx = DECL_CONTEXT (t);
>   	if (!(VAR_P (t) && DECL_LOCAL_DECL_P (t))
> @@ -15230,17 +15231,29 @@ 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);
> +		if (variable_template_p (gen_tmpl)
> +		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec))
> +		  {
> +		    /* We need to determine if we're using a partial
> +		       specialization now, because the type of the
> +		       variable could be different.  */
> +		    iloc_sentinel ils (saved_loc);
> +		    tree tid = build_nt (TEMPLATE_ID_EXPR, gen_tmpl, argvec);
> +		    partial_ti = most_specialized_partial_spec (tid, complain);
> +		    if (partial_ti == error_mark_node)
> +		      RETURN (error_mark_node);
> +		    else if (partial_ti)
> +		      {
> +			tree partial_tmpl = TI_TEMPLATE (partial_ti);
> +			tree partial_args = TI_ARGS (partial_ti);
> +			tree partial_pat = DECL_TEMPLATE_RESULT (partial_tmpl);
> +			t = partial_pat;
> +			args = partial_args;
> +			in_decl = partial_pat;
> +		      }
> +		  }
>   		hash = spec_hasher::hash (gen_tmpl, argvec);
>   		spec = retrieve_specialization (gen_tmpl, argvec, hash);

If you disagree with my suggestion above, the hash lookup should come 
before the partial specialization selection.

>   	      }
> @@ -15403,6 +15416,10 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
>   	      DECL_NOT_REALLY_EXTERN (r) = 1;
>   
>   	    DECL_TEMPLATE_INFO (r) = build_template_info (tmpl, argvec);
> +	    if (variable_template_p (tmpl))
> +	      /* Now that we we've formed this variable template specialization,
> +		 remember the result of most_specialized_partial_spec for it.  */
> +	      TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (r)) = partial_ti;
>   	    SET_DECL_IMPLICIT_INSTANTIATION (r);
>   	    if (!error_operand_p (r) || (complain & tf_error))
>   	      register_specialization (r, gen_tmpl, argvec, false, hash);
> @@ -22092,29 +22109,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
>   
>     tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
>   
> -  tree partial_ti = NULL_TREE;
> -  fndecl = NULL_TREE;
> -  if (VAR_P (pattern))
> -    {
> -      /* We need to determine if we're using a partial or explicit
> -	 specialization now, because the type of the variable could be
> -	 different.  */
> -      tree tid = build2 (TEMPLATE_ID_EXPR, NULL_TREE, tmpl, targ_ptr);
> -      partial_ti = most_specialized_partial_spec (tid, complain);
> -      if (partial_ti == error_mark_node)
> -	pattern = error_mark_node;
> -      else if (partial_ti)
> -	{
> -	  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);
> -	}
> -    }
> -
>     /* Substitute template parameters to obtain the specialization.  */
> -  if (fndecl == NULL_TREE)
> -    fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
> +  fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
>     if (DECL_CLASS_SCOPE_P (gen_tmpl))
>       pop_nested_class ();
>     pop_from_top_level ();
> @@ -22129,10 +22125,6 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
>        template, not the most general template.  */
>     DECL_TI_TEMPLATE (fndecl) = tmpl;
>     DECL_TI_ARGS (fndecl) = targ_ptr;
> -  if (VAR_P (pattern))
> -    /* Now that we we've formed this variable template specialization,
> -       remember the result of most_specialized_partial_spec for it.  */
> -    TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
>   
>     set_instantiating_module (fndecl);
>   

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

[Patrick Palka]

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

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
  * we're substituting a use of an already non-dependent variable
    template specialization

> 
> 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.  I wonder if we might want to do this for
alias template specializations too?

-- >8 --

gcc/cp/ChangeLog:

	* pt.cc (tsubst_decl) <case VAR_DECL>: Exit early for a
	non-dependent variable template specialization.  Don't
	look up or register a variable template specialization when
	called from instantiate_template.
	(instantiate_template): Register a variable template
	specialization here.
---
 gcc/cp/pt.cc | 53 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 2b2c598c3a9..3a9d43d383d 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -15192,6 +15192,14 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	if (!(VAR_P (t) && DECL_LOCAL_DECL_P (t))
 	    && (DECL_CLASS_SCOPE_P (t) || DECL_NAMESPACE_SCOPE_P (t)))
 	  {
+	    if (variable_template_specialization_p (t)
+		&& !uses_template_parms (INNERMOST_TEMPLATE_ARGS
+					 (DECL_TI_ARGS (t))))
+	      {
+		r = t;
+		break;
+	      }
+
 	    local_p = false;
 	    if (DECL_CLASS_SCOPE_P (t))
 	      {
@@ -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))))
+		  /* We're fully specializing a variable template via
+		     instantiate_template, which already attempted to look
+		     up the specialization, and will be also responsible for
+		     registering the specialization.  */;
+		else
+		  {
+		    argvec = tsubst (DECL_TI_ARGS (t), args, complain, in_decl);
+		    if (argvec == error_mark_node)
+		      RETURN (error_mark_node);
+		    hash = spec_hasher::hash (gen_tmpl, argvec);
+		    spec = retrieve_specialization (gen_tmpl, argvec, hash);
+		  }
 	      }
 	  }
 	else
@@ -15397,7 +15406,8 @@ 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 (argvec
+		&& (!error_operand_p (r) || (complain & tf_error)))
 	      register_specialization (r, gen_tmpl, argvec, false, hash);
 	  }
 	else
@@ -22032,7 +22042,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
 
-  /* 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);
+  spec = retrieve_specialization (gen_tmpl, targ_ptr, hash);
 
   gcc_checking_assert (tmpl == gen_tmpl
 		       || ((fndecl
@@ -22122,9 +22133,13 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
   DECL_TI_TEMPLATE (fndecl) = tmpl;
   DECL_TI_ARGS (fndecl) = targ_ptr;
   if (VAR_P (pattern))
-    /* Now that we we've formed this variable template specialization,
-       remember the result of most_specialized_partial_spec for it.  */
-    TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
+    {
+      /* Now that we we've formed this variable template specialization,
+	 remember the result of most_specialized_partial_spec for it.  */
+      TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
+      /* And register the specialization, which tsubst_decl deferred to us.  */
+      register_specialization (fndecl, gen_tmpl, targ_ptr, false, hash);
+    }
 
   set_instantiating_module (fndecl);
 
-- 
2.41.0.327.gaa9166bcc0


> 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.cc (tsubst_decl) <case VAR/TYPE_DECL>: Don't call
> > 	coerce_template_parms.  Call most_specialized_partial_spec
> > 	when fully specializing a variable template here ...
> > 	(instantiate_template): ... instead of here.  Always pass
> > 	the primary variable template pattern to tsubst_decl.
> > ---
> >   gcc/cp/pt.cc | 62 +++++++++++++++++++++++-----------------------------
> >   1 file changed, 27 insertions(+), 35 deletions(-)
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index fa15b75b9c5..53968b823d5 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -15194,6 +15194,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
> > complain)
> >   	/* Check to see if we already have the specialization we
> >   	   need.  */
> >   	tree spec = NULL_TREE;
> > +	tree partial_ti = NULL_TREE;
> >   	bool local_p = false;
> >   	tree ctx = DECL_CONTEXT (t);
> >   	if (!(VAR_P (t) && DECL_LOCAL_DECL_P (t))
> > @@ -15230,17 +15231,29 @@ 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);
> > +		if (variable_template_p (gen_tmpl)
> > +		    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec))
> > +		  {
> > +		    /* We need to determine if we're using a partial
> > +		       specialization now, because the type of the
> > +		       variable could be different.  */
> > +		    iloc_sentinel ils (saved_loc);
> > +		    tree tid = build_nt (TEMPLATE_ID_EXPR, gen_tmpl, argvec);
> > +		    partial_ti = most_specialized_partial_spec (tid,
> > complain);
> > +		    if (partial_ti == error_mark_node)
> > +		      RETURN (error_mark_node);
> > +		    else if (partial_ti)
> > +		      {
> > +			tree partial_tmpl = TI_TEMPLATE (partial_ti);
> > +			tree partial_args = TI_ARGS (partial_ti);
> > +			tree partial_pat = DECL_TEMPLATE_RESULT
> > (partial_tmpl);
> > +			t = partial_pat;
> > +			args = partial_args;
> > +			in_decl = partial_pat;
> > +		      }
> > +		  }
> >   		hash = spec_hasher::hash (gen_tmpl, argvec);
> >   		spec = retrieve_specialization (gen_tmpl, argvec, hash);
> 
> If you disagree with my suggestion above, the hash lookup should come before
> the partial specialization selection.

Makes sense.  Here's this updated approach for comparison:

gcc/cp/ChangeLog:

	* pt.cc (tsubst_decl) <case VAR/TYPE_DECL>: Don't call
	coerce_template_parms.  Call most_specialized_partial_spec
	when fully specializing a variable template here ...
	(instantiate_template): ... instead of here.  Always pass
	the primary variable template pattern to tsubst_decl.
---
 gcc/cp/pt.cc | 63 +++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index fa15b75b9c5..3987ffc509a 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -15194,6 +15194,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	/* Check to see if we already have the specialization we
 	   need.  */
 	tree spec = NULL_TREE;
+	tree partial_ti = NULL_TREE;
 	bool local_p = false;
 	tree ctx = DECL_CONTEXT (t);
 	if (!(VAR_P (t) && DECL_LOCAL_DECL_P (t))
@@ -15230,15 +15231,6 @@ 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);
@@ -15270,6 +15262,28 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	    break;
 	  }
 
+	if (gen_tmpl
+	    && variable_template_p (gen_tmpl)
+	    && TMPL_ARGS_DEPTH (args) >= TMPL_ARGS_DEPTH (argvec))
+	  {
+	    /* We need to determine if we're using a partial specialization
+	       now, because the type of the variable could be different.  */
+	    iloc_sentinel ils (saved_loc);
+	    tree tid = build_nt (TEMPLATE_ID_EXPR, gen_tmpl, argvec);
+	    partial_ti = most_specialized_partial_spec (tid, complain);
+	    if (partial_ti == error_mark_node)
+	      RETURN (error_mark_node);
+	    else if (partial_ti)
+	      {
+		tree partial_tmpl = TI_TEMPLATE (partial_ti);
+		tree partial_args = TI_ARGS (partial_ti);
+		tree partial_pat = DECL_TEMPLATE_RESULT (partial_tmpl);
+		t = partial_pat;
+		args = partial_args;
+		in_decl = partial_pat;
+	      }
+	  }
+
 	/* Create a new node for the specialization we need.  */
 	if (type == NULL_TREE)
 	  {
@@ -15403,6 +15417,10 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
 	      DECL_NOT_REALLY_EXTERN (r) = 1;
 
 	    DECL_TEMPLATE_INFO (r) = build_template_info (tmpl, argvec);
+	    if (variable_template_p (tmpl))
+	      /* Now that we we've formed this variable template specialization,
+		 remember the result of most_specialized_partial_spec for it.  */
+	      TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (r)) = partial_ti;
 	    SET_DECL_IMPLICIT_INSTANTIATION (r);
 	    if (!error_operand_p (r) || (complain & tf_error))
 	      register_specialization (r, gen_tmpl, argvec, false, hash);
@@ -22092,29 +22110,8 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
 
   tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
 
-  tree partial_ti = NULL_TREE;
-  fndecl = NULL_TREE;
-  if (VAR_P (pattern))
-    {
-      /* We need to determine if we're using a partial or explicit
-	 specialization now, because the type of the variable could be
-	 different.  */
-      tree tid = build2 (TEMPLATE_ID_EXPR, NULL_TREE, tmpl, targ_ptr);
-      partial_ti = most_specialized_partial_spec (tid, complain);
-      if (partial_ti == error_mark_node)
-	pattern = error_mark_node;
-      else if (partial_ti)
-	{
-	  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);
-	}
-    }
-
   /* Substitute template parameters to obtain the specialization.  */
-  if (fndecl == NULL_TREE)
-    fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
+  fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
   if (DECL_CLASS_SCOPE_P (gen_tmpl))
     pop_nested_class ();
   pop_from_top_level ();
@@ -22129,10 +22126,6 @@ instantiate_template (tree tmpl, tree orig_args, tsubst_flags_t complain)
      template, not the most general template.  */
   DECL_TI_TEMPLATE (fndecl) = tmpl;
   DECL_TI_ARGS (fndecl) = targ_ptr;
-  if (VAR_P (pattern))
-    /* Now that we we've formed this variable template specialization,
-       remember the result of most_specialized_partial_spec for it.  */
-    TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
 
   set_instantiating_module (fndecl);
 
-- 
2.41.0.327.gaa9166bcc0


> 
> >   	      }
> > @@ -15403,6 +15416,10 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
> > complain)
> >   	      DECL_NOT_REALLY_EXTERN (r) = 1;
> >     	    DECL_TEMPLATE_INFO (r) = build_template_info (tmpl, argvec);
> > +	    if (variable_template_p (tmpl))
> > +	      /* Now that we we've formed this variable template
> > specialization,
> > +		 remember the result of most_specialized_partial_spec for it.
> > */
> > +	      TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (r)) = partial_ti;
> >   	    SET_DECL_IMPLICIT_INSTANTIATION (r);
> >   	    if (!error_operand_p (r) || (complain & tf_error))
> >   	      register_specialization (r, gen_tmpl, argvec, false, hash);
> > @@ -22092,29 +22109,8 @@ instantiate_template (tree tmpl, tree orig_args,
> > tsubst_flags_t complain)
> >       tree pattern = DECL_TEMPLATE_RESULT (gen_tmpl);
> >   -  tree partial_ti = NULL_TREE;
> > -  fndecl = NULL_TREE;
> > -  if (VAR_P (pattern))
> > -    {
> > -      /* We need to determine if we're using a partial or explicit
> > -	 specialization now, because the type of the variable could be
> > -	 different.  */
> > -      tree tid = build2 (TEMPLATE_ID_EXPR, NULL_TREE, tmpl, targ_ptr);
> > -      partial_ti = most_specialized_partial_spec (tid, complain);
> > -      if (partial_ti == error_mark_node)
> > -	pattern = error_mark_node;
> > -      else if (partial_ti)
> > -	{
> > -	  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);
> > -	}
> > -    }
> > -
> >     /* Substitute template parameters to obtain the specialization.  */
> > -  if (fndecl == NULL_TREE)
> > -    fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
> > +  fndecl = tsubst (pattern, targ_ptr, complain, gen_tmpl);
> >     if (DECL_CLASS_SCOPE_P (gen_tmpl))
> >       pop_nested_class ();
> >     pop_from_top_level ();
> > @@ -22129,10 +22125,6 @@ instantiate_template (tree tmpl, tree orig_args,
> > tsubst_flags_t complain)
> >        template, not the most general template.  */
> >     DECL_TI_TEMPLATE (fndecl) = tmpl;
> >     DECL_TI_ARGS (fndecl) = targ_ptr;
> > -  if (VAR_P (pattern))
> > -    /* Now that we we've formed this variable template specialization,
> > -       remember the result of most_specialized_partial_spec for it.  */
> > -    TI_PARTIAL_INFO (DECL_TEMPLATE_INFO (fndecl)) = partial_ti;
> >       set_instantiating_module (fndecl);
> >   
> 
> 

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

[Jason Merrill]

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?

>    * 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.

> @@ -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.

Jason

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

[Patrick Palka]

[-- 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

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

[Jason Merrill]

On 7/17/23 17:29, Patrick Palka wrote:
> 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.

OK, thanks.

> -- >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