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

[Jason Merrill <jason@redhat.com>]

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