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

[Jason Merrill <jason@redhat.com>]

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