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.133.124]) by sourceware.org (Postfix) with ESMTPS id A86FE3858D28 for ; Mon, 17 Jul 2023 21:29:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A86FE3858D28 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=1689629394; 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: in-reply-to:in-reply-to:references:references; bh=N+HFxSiXRkJXMTUUIDk6hW6g8ryCeKHqfRQWANsS3dA=; b=NOOzue5TbUZ9JdMNc3AUwIRVXj+ypsAkUzGiWK0Q1kOSCdW1vivYiE83g8X+ENEszA7Ah9 5w3Z+YGeHJT1O/xJDe1jGGI2T6p2h44XfP2Li9Fd7LHm4Sp3UqRSf3HVOlPiifuMj5IZ4I KPiR2iof6XZROrdvQHTUhiI7RsBhXUs= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-618-rNTJkAKEPkq8O5Nd2K54qA-1; Mon, 17 Jul 2023 17:29:51 -0400 X-MC-Unique: rNTJkAKEPkq8O5Nd2K54qA-1 Received: by mail-qv1-f70.google.com with SMTP id 6a1803df08f44-63c566512c0so40262476d6.1 for ; Mon, 17 Jul 2023 14:29:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689629391; x=1692221391; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=wrMwnpBpVqe0J4x+ZGXOBNQYBuQx+ONmseYMwHrYKug=; b=K4JqZNYmz8n8tlshHy5Xycyr/SgBqxGv0/vIQw0/FJc2ToRTEuEBtxxRJzH873oiGD Vm0THWUv9NUnvk/BAvxfYu03ZE49Rp1cFcmroydMQFn6ZKoMThSY4ELFau7cSphehBRV riXNZaZXVLC0KFJgEfeKWWiwqbro2hMi/hZ2d8f3B7x49Utc1dacMl42L2p/Fn6xP3Bq 9LjhpWuMT4NT8d6dDjh7gm7xUDQuSppBDe4u9DpbHcma8IFk38CLURCpZhWe6QaDAsmA 4FPdvMfJMiKjEucaH6xZ9+FsdZVHdg2Jg80gIQCZDUu3Lg9f4oOnl6YGPXwobWV2Eby8 dA0w== X-Gm-Message-State: ABy/qLY9rH3f9aqNFaUJO/XHipieasWY0emVE2D0ls18evKStVvIwjL5 NqoRlmi7InMR6tAau4gHuQnvgOsXlaVuHNC4trhHfq3NRdxw4H52Cqrni+LjrWhYjyUjN+nU0cF I42kraLkx9WAn4oSVbw== X-Received: by 2002:ad4:5de6:0:b0:623:5c93:77eb with SMTP id jn6-20020ad45de6000000b006235c9377ebmr13976152qvb.13.1689629391137; Mon, 17 Jul 2023 14:29:51 -0700 (PDT) X-Google-Smtp-Source: APBJJlFXMEd7H9o4hNY0p5nDcrOvnrbcbH3fWBIyqY8l63radnCA5PlrA2elg9E9r94iFcmraeWqUw== X-Received: by 2002:ad4:5de6:0:b0:623:5c93:77eb with SMTP id jn6-20020ad45de6000000b006235c9377ebmr13976131qvb.13.1689629390704; Mon, 17 Jul 2023 14:29:50 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id q3-20020a0cf5c3000000b006364ae68f31sm196282qvm.97.2023.07.17.14.29.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Jul 2023 14:29:50 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Mon, 17 Jul 2023 17:29:49 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: redundant targ coercion for var/alias tmpls In-Reply-To: <5502feae-d5ef-092e-3f29-2b4fa9f38224@redhat.com> Message-ID: 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> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="8323329-1488008660-1689629390=:2236" X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,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: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1488008660-1689629390=:2236 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT 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 --8323329-1488008660-1689629390=:2236--