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