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 A43463829BEA for ; Fri, 10 Jun 2022 13:18:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A43463829BEA 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.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-670-ywQa9qd8MWiMeGhLCXdWrQ-1; Fri, 10 Jun 2022 09:18:10 -0400 X-MC-Unique: ywQa9qd8MWiMeGhLCXdWrQ-1 Received: by mail-qv1-f70.google.com with SMTP id kl30-20020a056214519e00b0046d8f1cd655so2727213qvb.19 for ; Fri, 10 Jun 2022 06:18:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=jKqakHU2MAB7HR5lL5myN+fJA1A/eRB1ILv1XyzR5Bk=; b=Dyih85m3B365q3kvgQy5kK5yA4RT1V6R2cNfFstFX35i+dplIkI5kK/koSSdbvbrCP iZXXPjRpfSliPxXboXi2mm5MR2TIPYI9C9OW3YF+kzNhFdySSmOyF+qJoL1GL5CIe5N/ GEYyBjab8V/r3zZ7+L6xGF34G1g1019rpLPZFgs4+6NqGQs9Z4N9eA1u0detOcNcpAZ0 CYYEtVzrMPvOGY4juYcLu0/53DxSu5FMsH0Q0qm705FYO55+ecBENRUmmDEkHEG9bC/k Wn5a5eRKY+GloSqKMTL/KhvIOe0Q9gqN5Us1IplLf0BjBklneLvEO+UxXkU/rlxzE+GG em8w== X-Gm-Message-State: AOAM532V2UlmNZJUmRiJedOQOFzPKdHjGa3Kho3xEKhJbFq5Am0ymZuJ f6xacCBgBXr9oNKaNbUY56BbQSNK3dBdmvIjDO4OgdZzGxyK6emw0oDpmcfZOInRRHsBbYOR62s ddv+pYZrF8bgBnVz20w== X-Received: by 2002:a05:622a:486:b0:304:ec30:8c0f with SMTP id p6-20020a05622a048600b00304ec308c0fmr21725966qtx.236.1654867089401; Fri, 10 Jun 2022 06:18:09 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyoV+YbwUz4meyLaO7rm9K0Mo1Xg7nHSPhY8DxiQqYeMODYIpCWGimXxjHuY4h4jC/GHGwPyA== X-Received: by 2002:a05:622a:486:b0:304:ec30:8c0f with SMTP id p6-20020a05622a048600b00304ec308c0fmr21725936qtx.236.1654867088935; Fri, 10 Jun 2022 06:18:08 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id m2-20020a05620a290200b006a74da327fcsm1266965qkp.99.2022.06.10.06.18.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jun 2022 06:18:08 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 10 Jun 2022 09:18:07 -0400 (EDT) To: Patrick Palka cc: Jason Merrill , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: optimize specialization of nested class templates In-Reply-To: Message-ID: <6a1b24b7-0f64-aa00-2717-91893e9d3261@idea> References: <20220608182147.4123587-1-ppalka@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-14.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jun 2022 13:18:14 -0000 On Thu, 9 Jun 2022, Patrick Palka wrote: > On Thu, 9 Jun 2022, Jason Merrill wrote: > > > On 6/8/22 14:21, Patrick Palka wrote: > > > When substituting a class template specialization, tsubst_aggr_type > > > substitutes the TYPE_CONTEXT before passing it to lookup_template_class. > > > This appears to be unnecessary, however, because the the initial value > > > of lookup_template_class's context parameter is unused outside of the > > > IDENTIFIER_NODE case, and l_t_c performs its own substitution of the > > > context, anyway. So this patch removes the redundant substitution in > > > tsubst_aggr_type. Doing so causes us to ICE on template/nested5.C > > > because during lookup_template_class for A::C::D with T=E and S=S, > > > we substitute and complete the context A::C with T=E, which in turn > > > registers the desired dependent specialization of D for us and we end up > > > trying to register it again. This patch fixes this by checking the > > > specializations table again after completion of the context. > > > > > > This patch also implements a couple of other optimizations: > > > > > > * In lookup_template_class, if the context of the partially > > > instantiated template is already non-dependent, then we could > > > reuse that instead of substituting the context of the most > > > general template. > > > * When substituting the TYPE_DECL for an injected-class-name > > > in tsubst_decl, we can avoid substituting its TREE_TYPE and > > > DECL_TI_ARGS. > > > > > > Together these optimizations improve memory usage for the range-v3 > > > testcase test/view/split.cc by about 5%. The improvement is probably > > > more significant when dealing with deeply nested class templates. > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > > trunk? > > > > > > gcc/cp/ChangeLog: > > > > > > * pt.cc (lookup_template_class): Remove dead stores to > > > context parameter. Don't substitute the context of the > > > most general template if that of the partially instantiated > > > template is non-dependent. Check the specializations table > > > again after completing the context of a nested dependent > > > specialization. > > > (tsubst_aggr_type) : Don't substitute > > > TYPE_CONTEXT or pass it to lookup_template_class. > > > (tsubst_decl) : Avoid substituting the > > > TREE_TYPE and DECL_TI_ARGS when DECL_SELF_REFERENCE_P. > > > --- > > > gcc/cp/pt.cc | 69 +++++++++++++++++++++++++++++++--------------------- > > > 1 file changed, 41 insertions(+), 28 deletions(-) > > > > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > > index 59b94317e88..28023d60684 100644 > > > --- a/gcc/cp/pt.cc > > > +++ b/gcc/cp/pt.cc > > > @@ -9840,8 +9840,6 @@ lookup_template_class (tree d1, tree arglist, tree > > > in_decl, tree context, > > > if (context) > > > pop_decl_namespace (); > > > } > > > - if (templ) > > > - context = DECL_CONTEXT (templ); > > > } > > > else if (TREE_CODE (d1) == TYPE_DECL && MAYBE_CLASS_TYPE_P (TREE_TYPE > > > (d1))) > > > { > > > @@ -9868,7 +9866,6 @@ lookup_template_class (tree d1, tree arglist, tree > > > in_decl, tree context, > > > { > > > templ = d1; > > > d1 = DECL_NAME (templ); > > > - context = DECL_CONTEXT (templ); > > > } > > > else if (DECL_TEMPLATE_TEMPLATE_PARM_P (d1)) > > > { > > > @@ -10059,8 +10056,25 @@ lookup_template_class (tree d1, tree arglist, tree > > > in_decl, tree context, > > > context = DECL_CONTEXT (gen_tmpl); > > > if (context && TYPE_P (context)) > > > { > > > - context = tsubst_aggr_type (context, arglist, complain, in_decl, > > > true); > > > - context = complete_type (context); > > > + if (!uses_template_parms (DECL_CONTEXT (templ))) > > > + /* If the context of the partially instantiated template is > > > + already non-dependent, then we might as well use it. */ > > > + context = DECL_CONTEXT (templ); > > > + else > > > + { > > > + context = tsubst_aggr_type (context, arglist, complain, in_decl, > > > true); > > > + context = complete_type (context); > > > + if (is_dependent_type && arg_depth > 1) > > > + { > > > + /* If this is a dependent nested specialization such as > > > + A::B, then completion of A might have > > > + registered this specialization of B for us, so check > > > + the table again (33959). */ > > > + entry = type_specializations->find_with_hash (&elt, hash); > > > + if (entry) > > > + return entry->spec; > > > + } > > > + } > > > } > > > else > > > context = tsubst (context, arglist, complain, in_decl); > > > @@ -13711,25 +13725,12 @@ tsubst_aggr_type (tree t, > > > if (TYPE_TEMPLATE_INFO (t) && uses_template_parms (t)) > > > { > > > tree argvec; > > > - tree context; > > > tree r; > > > /* In "sizeof(X)" we need to evaluate "I". */ > > > cp_evaluated ev; > > > - /* First, determine the context for the type we are looking > > > - up. */ > > > - context = TYPE_CONTEXT (t); > > > - if (context && TYPE_P (context)) > > > - { > > > - context = tsubst_aggr_type (context, args, complain, > > > - in_decl, /*entering_scope=*/1); > > > - /* If context is a nested class inside a class template, > > > - it may still need to be instantiated (c++/33959). */ > > > - context = complete_type (context); > > > - } > > > - > > > - /* Then, figure out what arguments are appropriate for the > > > + /* Figure out what arguments are appropriate for the > > > type we are trying to find. For example, given: > > > template struct S; > > > @@ -13744,7 +13745,7 @@ tsubst_aggr_type (tree t, > > > r = error_mark_node; > > > else > > > { > > > - r = lookup_template_class (t, argvec, in_decl, context, > > > + r = lookup_template_class (t, argvec, in_decl, NULL_TREE, > > > entering_scope, complain); > > > r = cp_build_qualified_type (r, cp_type_quals (t), complain); > > > } > > > @@ -14880,6 +14881,10 @@ tsubst_decl (tree t, tree args, tsubst_flags_t > > > complain) > > > ctx = tsubst_aggr_type (ctx, args, > > > complain, > > > in_decl, /*entering_scope=*/1); > > > + if (DECL_SELF_REFERENCE_P (t)) > > > + /* The context and type of a injected-class-name are > > > + the same, so we don't need to substitute both. */ > > > + type = ctx; > > > /* If CTX is unchanged, then T is in fact the > > > specialization we want. That situation occurs when > > > referencing a static data member within in its own > > > @@ -14900,14 +14905,22 @@ 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) > > > - argvec = (coerce_innermost_template_parms > > > - (DECL_TEMPLATE_PARMS (gen_tmpl), > > > - argvec, t, complain, > > > - /*all*/true, /*defarg*/true)); > > > - if (argvec == error_mark_node) > > > - RETURN (error_mark_node); > > > + if (DECL_SELF_REFERENCE_P (t)) > > > + /* The DECL_TI_ARGS for the injected-class-name are the > > > + generic template arguments for the class template, so > > > + substitution/coercion is just the identity mapping. */ > > > + argvec = args; > > > > Would it make sense to extend this to any TEMPLATE_DECL for which > > DECL_PRIMARY_TEMPLATE is the class template? So, anything that gets here > > except an alias template. > > Ah nice, it does look like we could extend this to any TEMPLATE_DECL that > satisfies DECL_CLASS_SCOPE_P && !DECL_MEMBER_TEMPLATE_P (so that we also > exclude static data member templates), i.e. any templated non-template > member. Is that equivalent to what you had in mind? > > Based on some light testing, if we do this, it seems we also need to > handle 'args' having greater depth than 'DECL_TI_ARGS' here, something > which could happen during satisfaction. ... or we could just restrict the optimization to when the argument depths match, like so: -- >8 -- Subject: [PATCH] c++: optimize specialization of nested templated classes gcc/cp/ChangeLog: * pt.cc (lookup_template_class): Remove dead stores to context parameter. Don't substitute the context of the most general template if that of the partially instantiated template is non-dependent. Check the specializations table again after completing the context of a nested dependent specialization. (tsubst_aggr_type) : Don't substitute TYPE_CONTEXT or pass it to lookup_template_class. (tsubst_decl) : Avoid substituting the TREE_TYPE and DECL_TI_ARGS when DECL_SELF_REFERENCE_P. --- gcc/cp/pt.cc | 73 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 3154186ac20..ebd822373db 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -9840,8 +9840,6 @@ lookup_template_class (tree d1, tree arglist, tree in_decl, tree context, if (context) pop_decl_namespace (); } - if (templ) - context = DECL_CONTEXT (templ); } else if (TREE_CODE (d1) == TYPE_DECL && MAYBE_CLASS_TYPE_P (TREE_TYPE (d1))) { @@ -9868,7 +9866,6 @@ lookup_template_class (tree d1, tree arglist, tree in_decl, tree context, { templ = d1; d1 = DECL_NAME (templ); - context = DECL_CONTEXT (templ); } else if (DECL_TEMPLATE_TEMPLATE_PARM_P (d1)) { @@ -10059,8 +10056,26 @@ lookup_template_class (tree d1, tree arglist, tree in_decl, tree context, context = DECL_CONTEXT (gen_tmpl); if (context && TYPE_P (context)) { - context = tsubst_aggr_type (context, arglist, complain, in_decl, true); - context = complete_type (context); + if (!uses_template_parms (DECL_CONTEXT (templ))) + /* If the context of the partially instantiated template is + already non-dependent, then we might as well use it. */ + context = DECL_CONTEXT (templ); + else + { + context = tsubst_aggr_type (context, arglist, + complain, in_decl, true); + context = complete_type (context); + if (is_dependent_type && arg_depth > 1) + { + /* If this is a dependent nested specialization such as + A::B, then completion of A might have + registered this specialization of B for us, so check + the table again (33959). */ + entry = type_specializations->find_with_hash (&elt, hash); + if (entry) + return entry->spec; + } + } } else context = tsubst (context, arglist, complain, in_decl); @@ -13739,25 +13754,12 @@ tsubst_aggr_type (tree t, if (TYPE_TEMPLATE_INFO (t) && uses_template_parms (t)) { tree argvec; - tree context; tree r; /* In "sizeof(X)" we need to evaluate "I". */ cp_evaluated ev; - /* First, determine the context for the type we are looking - up. */ - context = TYPE_CONTEXT (t); - if (context && TYPE_P (context)) - { - context = tsubst_aggr_type (context, args, complain, - in_decl, /*entering_scope=*/1); - /* If context is a nested class inside a class template, - it may still need to be instantiated (c++/33959). */ - context = complete_type (context); - } - - /* Then, figure out what arguments are appropriate for the + /* Figure out what arguments are appropriate for the type we are trying to find. For example, given: template struct S; @@ -13772,7 +13774,7 @@ tsubst_aggr_type (tree t, r = error_mark_node; else { - r = lookup_template_class (t, argvec, in_decl, context, + r = lookup_template_class (t, argvec, in_decl, NULL_TREE, entering_scope, complain); r = cp_build_qualified_type (r, cp_type_quals (t), complain); } @@ -14913,6 +14915,10 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain) ctx = tsubst_aggr_type (ctx, args, complain, in_decl, /*entering_scope=*/1); + if (DECL_SELF_REFERENCE_P (t)) + /* The context and type of an injected-class-name are + the same, so we don't need to substitute both. */ + type = ctx; /* If CTX is unchanged, then T is in fact the specialization we want. That situation occurs when referencing a static data member within in its own @@ -14933,14 +14939,25 @@ 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) - argvec = (coerce_innermost_template_parms - (DECL_TEMPLATE_PARMS (gen_tmpl), - argvec, t, complain, - /*all*/true, /*defarg*/true)); - if (argvec == error_mark_node) - RETURN (error_mark_node); + if (DECL_CLASS_SCOPE_P (tmpl) + && !DECL_MEMBER_TEMPLATE_P (tmpl) + && (TMPL_ARGS_DEPTH (args) + == TMPL_ARGS_DEPTH (DECL_TI_ARGS (t)))) + /* The DECL_TI_ARGS in this case are the generic template + arguments for the class template, so substitution/coercion + is just the identity mapping. */ + argvec = args; + else + { + argvec = tsubst (DECL_TI_ARGS (t), args, complain, in_decl); + if (argvec != error_mark_node) + argvec = (coerce_innermost_template_parms + (DECL_TEMPLATE_PARMS (gen_tmpl), + argvec, t, complain, + /*all*/true, /*defarg*/true)); + if (argvec == error_mark_node) + RETURN (error_mark_node); + } hash = hash_tmpl_and_args (gen_tmpl, argvec); spec = retrieve_specialization (gen_tmpl, argvec, hash); } -- 2.36.1.363.g9c897eef06