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 77DA3384D1BA for ; Thu, 6 Oct 2022 04:17:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 77DA3384D1BA 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=1665029857; 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=kNJ69n84xHvZE1aY3aKUxUDYPxfLZA6sHHSy6S8obV4=; b=XwhKcFbve9vygUk11HG4cFDhlUDbG/mWx4tUu6GmBfqTC1BiRBE4AVJYNm39qOBbi8CuYl FnZlvx9dFWD9wNAD4QQj1HdM8cZOPTHe7nh4FjFehmmTzgJSKw0reIvWLMh5SBrZkAgmYA UF0ZmiC7SHUCtwoccUdtCe3SyATnWJY= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-463-2g7CIr7hMPOK3J73-6RxGg-1; Thu, 06 Oct 2022 00:17:34 -0400 X-MC-Unique: 2g7CIr7hMPOK3J73-6RxGg-1 Received: by mail-qt1-f197.google.com with SMTP id e22-20020ac84b56000000b0035bb64ad562so413811qts.17 for ; Wed, 05 Oct 2022 21:17:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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; bh=kNJ69n84xHvZE1aY3aKUxUDYPxfLZA6sHHSy6S8obV4=; b=vGA5jaluheF+Yt4+3WbX2ReAHLR1RF9nlDF1bVPS6wQU5wJbiO7JOEuzkmz5lllI1J OWUOB2nv2qyHkGj5iVw1Xn90MNVic8OR5d5jFQc3ZlmfA3MZDNDObw5QCg+sJuc9OYyU e3An8sow6+egWZURthGy+aCRXrtvw5uR82iyoMPrvtJkg0iD+1OxOVC4EAwS4r6Tr9Xn /TGJkltLn9RVzg+D39QRdQ7inu5AC8xiwJeRGgF246dZynMPE6tcijhxfyjj4cebI/VR xOfXbd/EDl7yzrI7FX8WF9sui3fnXH0EyBQrCY1WNmUwLhxHOKOH3FWcdcpw6mHa7x3b Cynw== X-Gm-Message-State: ACrzQf014w+4RCHrt9Fp66HCmkTt0jNOlJKQZehS1TOK9TRcp+AsR7Dx P61TzpduPJU8BGC+lpMQJz0hlB4+Ujd5e48JKUlYyFipgwsvtZgDbS9bJsDqFxz5EwSWOWgOg2J G1bhF4DpZpACaSPn52Q== X-Received: by 2002:a05:620a:1a0c:b0:6cb:e0a3:f889 with SMTP id bk12-20020a05620a1a0c00b006cbe0a3f889mr1948929qkb.538.1665029853294; Wed, 05 Oct 2022 21:17:33 -0700 (PDT) X-Google-Smtp-Source: AMsMyM40iNkbMkSg0YXbw8EBUGLZtgNvfsXoYKYxvZ5lPXCh4f7S1tb7Fh2d6b4yp1TVgRPFe/kwZw== X-Received: by 2002:a05:620a:1a0c:b0:6cb:e0a3:f889 with SMTP id bk12-20020a05620a1a0c00b006cbe0a3f889mr1948917qkb.538.1665029852884; Wed, 05 Oct 2022 21:17:32 -0700 (PDT) Received: from [192.168.1.101] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id o13-20020a05620a2a0d00b006ce51b541dfsm19390370qkp.36.2022.10.05.21.17.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 05 Oct 2022 21:17:32 -0700 (PDT) Message-ID: <16afa903-5f30-8c2f-1fb0-950004722948@redhat.com> Date: Thu, 6 Oct 2022 00:17:30 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [PATCH] c++: remove optimize_specialization_lookup_p To: Patrick Palka , gcc-patches@gcc.gnu.org Cc: nathan@acm.org References: <20221006020226.3629040-1-ppalka@redhat.com> From: Jason Merrill In-Reply-To: <20221006020226.3629040-1-ppalka@redhat.com> 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: 7bit X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP 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 10/5/22 22:02, Patrick Palka wrote: > Roughly speaking, optimize_specialization_lookup_p returns true > for a non-template member function of a class template, e.g. > > template struct A { int f(); }; > > The idea behind the optimization in question seems to be that if we want > to look up the specialization A::f [with T=int], then we can just do > a name lookup for f in A and avoid having to maintain a spec_entry > for it in the decl_specializations table. > > However, the benefit of this optimization seems questionable because in > order to do the name lookup we need to look up A [with T=int] in the > type_specializations table, which is just as expensive as looking up an > entry in decl_specializations. And according to my experiments using > stdc++.h, range-v3 and libstdc++ tests, the compiler is slightly (<1%) > _faster_ after disabling this optimization! Yeah, probably should have done this when we switched to hash tables. > Additionally, this optimization means that an explicit specialization > won't get recorded in decl_specializations for such a template, which > is a rather arbitrary invariant violation, and apparently breaks the > below modules testcase. > > So since this optimization doesn't seem to give a performance benefit, > complicates the explicit specialization story, and affects modules, this > patch proposes to remove it. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk? Patch generated with -w to suppress noisy whitespace changes. OK. > gcc/cp/ChangeLog: > > * pt.cc (optimize_specialization_lookup_p): Remove. > (retrieve_specialization): Assume the above returns false > and simplify accordingly. > (register_specialization): Likewise. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/indirect-3_b.C: Expect that the entity > foo::TPL<0>::frob has is tagged as a specialization and > not just a declaration. > * g++.dg/modules/tpl-spec-8_a.H: New test. > * g++.dg/modules/tpl-spec-8_b.C: New test. > --- > gcc/cp/pt.cc | 85 +-------------------- > gcc/testsuite/g++.dg/modules/indirect-3_b.C | 2 +- > gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H | 9 +++ > gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C | 8 ++ > 4 files changed, 22 insertions(+), 82 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index bce2a777922..c079bbd4268 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -1170,39 +1170,6 @@ maybe_process_partial_specialization (tree type) > return type; > } > > -/* Returns nonzero if we can optimize the retrieval of specializations > - for TMPL, a TEMPLATE_DECL. In particular, for such a template, we > - do not use DECL_TEMPLATE_SPECIALIZATIONS at all. */ > - > -static inline bool > -optimize_specialization_lookup_p (tree tmpl) > -{ > - return (DECL_FUNCTION_TEMPLATE_P (tmpl) > - && DECL_CLASS_SCOPE_P (tmpl) > - /* DECL_CLASS_SCOPE_P holds of T::f even if T is a template > - parameter. */ > - && CLASS_TYPE_P (DECL_CONTEXT (tmpl)) > - /* The optimized lookup depends on the fact that the > - template arguments for the member function template apply > - purely to the containing class, which is not true if the > - containing class is an explicit or partial > - specialization. */ > - && !CLASSTYPE_TEMPLATE_SPECIALIZATION (DECL_CONTEXT (tmpl)) > - && !DECL_MEMBER_TEMPLATE_P (tmpl) > - && !DECL_CONV_FN_P (tmpl) > - /* It is possible to have a template that is not a member > - template and is not a member of a template class: > - > - template > - struct S { friend A::f(); }; > - > - Here, the friend function is a template, but the context does > - not have template information. The optimized lookup relies > - on having ARGS be the template arguments for both the class > - and the function template. */ > - && !DECL_UNIQUE_FRIEND_P (DECL_TEMPLATE_RESULT (tmpl))); > -} > - > /* Make sure ARGS doesn't use any inappropriate typedefs; we should have > gone through coerce_template_parms by now. */ > > @@ -1276,43 +1243,12 @@ retrieve_specialization (tree tmpl, tree args, hashval_t hash) > if (lambda_fn_in_template_p (tmpl)) > return NULL_TREE; > > - if (optimize_specialization_lookup_p (tmpl)) > - { > - /* The template arguments actually apply to the containing > - class. Find the class specialization with those > - arguments. */ > - tree class_template = CLASSTYPE_TI_TEMPLATE (DECL_CONTEXT (tmpl)); > - tree class_specialization > - = retrieve_specialization (class_template, args, 0); > - if (!class_specialization) > - return NULL_TREE; > - > - /* Find the instance of TMPL. */ > - tree fns = get_class_binding (class_specialization, DECL_NAME (tmpl)); > - for (ovl_iterator iter (fns); iter; ++iter) > - { > - tree fn = *iter; > - if (tree ti = get_template_info (fn)) > - if (TI_TEMPLATE (ti) == tmpl > - /* using-declarations can bring in a different > - instantiation of tmpl as a member of a different > - instantiation of tmpl's class. We don't want those > - here. */ > - && DECL_CONTEXT (fn) == class_specialization) > - return fn; > - } > - return NULL_TREE; > - } > - else > - { > - spec_entry *found; > spec_entry elt; > - spec_hash_table *specializations; > - > elt.tmpl = tmpl; > elt.args = args; > elt.spec = NULL_TREE; > > + spec_hash_table *specializations; > if (DECL_CLASS_TEMPLATE_P (tmpl)) > specializations = type_specializations; > else > @@ -1320,10 +1256,8 @@ retrieve_specialization (tree tmpl, tree args, hashval_t hash) > > if (hash == 0) > hash = spec_hasher::hash (&elt); > - found = specializations->find_with_hash (&elt, hash); > - if (found) > + if (spec_entry *found = specializations->find_with_hash (&elt, hash)) > return found->spec; > - } > > return NULL_TREE; > } > @@ -1567,8 +1501,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend, > hashval_t hash) > { > tree fn; > - spec_entry **slot = NULL; > - spec_entry elt; > > gcc_assert ((TREE_CODE (tmpl) == TEMPLATE_DECL && DECL_P (spec)) > || (TREE_CODE (tmpl) == FIELD_DECL > @@ -1589,12 +1521,7 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend, > instantiation unless and until it is actually needed. */ > return spec; > > - if (optimize_specialization_lookup_p (tmpl)) > - /* We don't put these specializations in the hash table, but we might > - want to give an error about a mismatch. */ > - fn = retrieve_specialization (tmpl, args, 0); > - else > - { > + spec_entry elt; > elt.tmpl = tmpl; > elt.args = args; > elt.spec = spec; > @@ -1602,12 +1529,11 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend, > if (hash == 0) > hash = spec_hasher::hash (&elt); > > - slot = decl_specializations->find_slot_with_hash (&elt, hash, INSERT); > + spec_entry **slot = decl_specializations->find_slot_with_hash (&elt, hash, INSERT); > if (*slot) > fn = (*slot)->spec; > else > fn = NULL_TREE; > - } > > /* We can sometimes try to re-register a specialization that we've > already got. In particular, regenerate_decl_from_template calls > @@ -1704,8 +1630,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend, > && !check_specialization_namespace (tmpl)) > DECL_CONTEXT (spec) = DECL_CONTEXT (tmpl); > > - if (slot != NULL /* !optimize_specialization_lookup_p (tmpl) */) > - { > spec_entry *entry = ggc_alloc (); > gcc_assert (tmpl && args && spec); > *entry = elt; > @@ -1723,7 +1647,6 @@ register_specialization (tree spec, tree tmpl, tree args, bool is_friend, > specialization would have used it. */ > DECL_TEMPLATE_INSTANTIATIONS (tmpl) > = tree_cons (args, spec, DECL_TEMPLATE_INSTANTIATIONS (tmpl)); > - } > > return spec; > } > diff --git a/gcc/testsuite/g++.dg/modules/indirect-3_b.C b/gcc/testsuite/g++.dg/modules/indirect-3_b.C > index 5bdfc1d36a8..038b01ecab7 100644 > --- a/gcc/testsuite/g++.dg/modules/indirect-3_b.C > +++ b/gcc/testsuite/g++.dg/modules/indirect-3_b.C > @@ -23,7 +23,7 @@ namespace bar > // { dg-final { scan-lang-dump {Lazily binding '::foo@foo:.::TPL'@'foo' section} module } } > // { dg-final { scan-lang-dump {Wrote import:-[0-9]* template_decl:'::foo@foo:.::template TPL@foo:.'@foo} module } } > > -// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::TPL<0x0>'\n \[1\]=specialization declaration '::foo@foo:.::TPL<0x0>::TPL<0x0>'\n( \[.\]=[^\n]* '\n)* \[.\]=decl definition '::foo@foo:.::TPL<0x0>::frob<0x0>'\n} module } } > +// { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::TPL<0x0>'\n \[1\]=specialization definition '::foo@foo:.::TPL<0x0>::frob<0x0>'\n \[2\]=specialization declaration '::foo@foo:.::TPL<0x0>::TPL<0x0>'} module } } > > // { dg-final { scan-lang-dump {Cluster members:\n \[0\]=specialization definition '::foo@foo:.::X@foo:.::frob<0x0>'} module } } > // { dg-final { scan-lang-dump {Writing:-[0-9]*'s type spec merge key \(specialization\) type_decl:'::foo@foo:.::TPL<0x0>'} module } } > diff --git a/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H b/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H > new file mode 100644 > index 00000000000..5f85556d7d7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/tpl-spec-8_a.H > @@ -0,0 +1,9 @@ > +// { dg-additional-options -fmodule-header } > +// { dg-module-cmi {} } > + > +template > +struct A { > + static void f() { T::nonexistent; } > +}; > + > +template<> inline void A::f() { } > diff --git a/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C b/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C > new file mode 100644 > index 00000000000..f23eb370370 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/modules/tpl-spec-8_b.C > @@ -0,0 +1,8 @@ > +// { dg-additional-options -fmodules-ts } > +// { dg-do link } > + > +import "tpl-spec-8_a.H"; > + > +int main() { > + A::f(); > +}