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

[Jason Merrill <jason@redhat.com>]

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