From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 3612A385AFB4 for ; Tue, 18 Jul 2023 16:52:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3612A385AFB4 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689699130; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eKymsbw2LbPjWF00/WgjJ4GYQaGj0DOZhviKEzPlxXg=; b=jTlzFmA+dr/nGhXZtHX0xi04teu5fQMS2TrupaDy1Osjkdxb7t0a0Wo/JSlQyvu2rs9Agh DpCXD/ASL9vM2FAZ5iP0EBDFLbCwJ9UlC2UZYcAserH65Mm5f7fnH32qdravVz0J0OpOtm jRK3hkek8XM2dgIoTle6Nede38Gao48= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-125-EjhKIsSCNqeTJrrrBUbNbQ-1; Tue, 18 Jul 2023 12:52:09 -0400 X-MC-Unique: EjhKIsSCNqeTJrrrBUbNbQ-1 Received: by mail-qk1-f198.google.com with SMTP id af79cd13be357-76783ba6d62so681709185a.3 for ; Tue, 18 Jul 2023 09:52:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689699128; x=1692291128; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=eKymsbw2LbPjWF00/WgjJ4GYQaGj0DOZhviKEzPlxXg=; b=iJUicDbl0n7oGxCCoVRAP20jACdHuBqHFUxNZVje5JJe4MffuHkkwmTGAHFCO5n6J2 jpzvReRI+cRaMSTa7aMC53KASRrVwq1wu8tUZYXYvQRJR01w0IFtrsT+QJXAWb1vSzeg ZXZKXnwEzYhCZUrMez5ZLpIW0ttnfMrxUKrjdfvO/NH1lUkVs3of7jWNulLBCLBpucfV wXKGXpQoYhuXtS9YOp38DFdolKVvzSwdxctSGe0FFSeCfT5dlQWvpXgmtBJ/0gfQc/Fa a8xMYSuYja9XWQfjUvsk3EDkXjNYVT3ZGNTIxL5SOyjR6kULs/qYb4izuPKDHaXpSbK0 xtLw== X-Gm-Message-State: ABy/qLZ+NT1onohrgs7Zb1XWOw629CiN2kyTug8gtIz6WJJvg1wq7Gyw tU4FDNFx6swDlw5kVvuA1h5XczFimpwTiP2j8E7GgE7Ji61LpQSIzi3k6npc4dudZjaOfyAc6Eo kYfPcf/TwLzD7wIF6kkA6vLn4/Q== X-Received: by 2002:a05:620a:4144:b0:767:7d86:b7fe with SMTP id k4-20020a05620a414400b007677d86b7femr3566136qko.6.1689699128565; Tue, 18 Jul 2023 09:52:08 -0700 (PDT) X-Google-Smtp-Source: APBJJlFE9hqUoyRQ+EC3JxdL2va6SYAo3sVHCSJt3yXUVSZE130GptGsq+PRHOTJiXMXvMhPTyTNmg== X-Received: by 2002:a05:620a:4144:b0:767:7d86:b7fe with SMTP id k4-20020a05620a414400b007677d86b7femr3566115qko.6.1689699128074; Tue, 18 Jul 2023 09:52:08 -0700 (PDT) Received: from [192.168.1.108] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id ou30-20020a05620a621e00b0075941df3365sm715774qkn.52.2023.07.18.09.52.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jul 2023 09:52:07 -0700 (PDT) Message-ID: <1ac00b10-bef7-7f35-9da1-5a27b4a5f15c@redhat.com> Date: Tue, 18 Jul 2023 12:52:06 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH] c++: redundant targ coercion for var/alias tmpls To: Patrick Palka Cc: gcc-patches@gcc.gnu.org References: <20230621171920.1283054-1-ppalka@redhat.com> <94b29e42-dfa9-5c05-4f1d-3c5beb998fdf@redhat.com> <26258d18-5718-9387-087b-be58a25048c2@redhat.com> <4388a939-94bd-048c-f234-e54bab92d937@idea> <9e51de65-9910-445b-1ec7-8a3cd47114fc@idea> <5502feae-d5ef-092e-3f29-2b4fa9f38224@redhat.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 >>>>>> 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. 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. > : 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