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 > > > > > 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 struct A { template static int v; }; template struct A; 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. : Pass 'use_spec_table' to tsubst_function_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