From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id E7FC53858C74; Fri, 26 Aug 2022 11:32:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E7FC53858C74 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id B697C1F8E5; Fri, 26 Aug 2022 11:32:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1661513578; h=from:from:reply-to: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=C1JgGyriNOZ4pig5P0J0XXakTg9CFFnXnG2uGd7Bpq8=; b=WBcE552unawCxfQC4gNWZ43OA2cBMYVymmWWKTiKipmVGiXlKQRkQhByJKMD2AMSLOBruO 1HeYUIfkz2Nx6aA9B2b2gEt9I0O1Fw35HsdYf73rGboXaJBE+tSOgNDYlDWIcSncgiKTfx pKV7rcDXdW8/S1hm4tngotdlTVx9Ulg= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1661513578; h=from:from:reply-to: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=C1JgGyriNOZ4pig5P0J0XXakTg9CFFnXnG2uGd7Bpq8=; b=/L7TQFQKi5xGMYWED5eiyQWP451R3lo006df2BVuEqUXBvH5ntxh1JvcM+y36+p4Ou2BOm Ekpd2eyTsVjfTcAg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id A2E2113421; Fri, 26 Aug 2022 11:32:58 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id GKC8J2qvCGOOeAAAMHmgww (envelope-from ); Fri, 26 Aug 2022 11:32:58 +0000 From: Martin Jambor To: Alexander Monakov , gcc-patches@gcc.gnu.org Cc: Alexander Monakov , Jan Hubicka , Artem Klimov Subject: Re: [PATCH v2] ipa-visibility: Optimize TLS access [PR99619] In-Reply-To: <20220707155341.5884-1-amonakov@ispras.ru> References: <20220707155341.5884-1-amonakov@ispras.ru> User-Agent: Notmuch/0.35 (https://notmuchmail.org) Emacs/28.1 (x86_64-suse-linux-gnu) Date: Fri, 26 Aug 2022 13:32:58 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,SPF_HELO_NONE,SPF_SOFTFAIL,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: Hi, sorry for ignoring this for so long, I hope that Honza wold take over. I think the patch would be good if it did not have.... On Thu, Jul 07 2022, Alexander Monakov via Gcc-patches wrote: > From: Artem Klimov > > Fix PR99619, which asks to optimize TLS model based on visibility. > The fix is implemented as an IPA optimization: this allows to take > optimized visibility status into account (as well as avoid modifying > all language frontends). > > 2022-04-17 Artem Klimov > > gcc/ChangeLog: > > * ipa-visibility.cc (function_and_variable_visibility): Promote > TLS access model afer visibility optimizations. > * varasm.cc (have_optimized_refs): New helper. > (optimize_dyn_tls_for_decl_p): New helper. Use it ... > (decl_default_tls_model): ... here in place of 'optimize' check. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tls/vis-attr-gd.c: New test. > * gcc.dg/tls/vis-attr-hidden-gd.c: New test. > * gcc.dg/tls/vis-attr-hidden.c: New test. > * gcc.dg/tls/vis-flag-hidden-gd.c: New test. > * gcc.dg/tls/vis-flag-hidden.c: New test. > * gcc.dg/tls/vis-pragma-hidden-gd.c: New test. > * gcc.dg/tls/vis-pragma-hidden.c: New test. > > Co-Authored-By: Alexander Monakov > Signed-off-by: Artem Klimov > --- > > v2: run the new loop in ipa-visibility only in the whole-program IPA pass; > in decl_default_tls_model, check if any referring function is optimized > when 'optimize == 0' (when running in LTO mode) > > > Note for reviewers: I noticed there's a place which tries to avoid TLS > promotion, but the comment seems wrong and I could not find a testcase. > I'd suggest we remove it. The compiler can only promote general-dynamic > to local-dynamic and initial-exec to local-exec. The comment refers to > promoting x-dynamic to y-exec, but that cannot happen AFAICT: > https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=8e1ba78f1b8eedd6c65c6f0e6d6d09a801de5d3d > > > gcc/ipa-visibility.cc | 19 +++++++++++ > gcc/testsuite/gcc.dg/tls/vis-attr-gd.c | 12 +++++++ > gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c | 13 ++++++++ > gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c | 12 +++++++ > gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c | 13 ++++++++ > gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c | 12 +++++++ > .../gcc.dg/tls/vis-pragma-hidden-gd.c | 17 ++++++++++ > gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c | 16 ++++++++++ > gcc/varasm.cc | 32 ++++++++++++++++++- > 9 files changed, 145 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-attr-gd.c > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c > create mode 100644 gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c > > diff --git a/gcc/ipa-visibility.cc b/gcc/ipa-visibility.cc > index 8a27e7bcd..3ed2b7cf6 100644 > --- a/gcc/ipa-visibility.cc > +++ b/gcc/ipa-visibility.cc > @@ -873,6 +873,25 @@ function_and_variable_visibility (bool whole_program) > } > } > > + if (symtab->state >= IPA_SSA) > + { > + FOR_EACH_VARIABLE (vnode) > + { > + tree decl = vnode->decl; > + > + /* Upgrade TLS access model based on optimized visibility status, > + unless it was specified explicitly or no references remain. */ > + if (DECL_THREAD_LOCAL_P (decl) > + && !lookup_attribute ("tls_model", DECL_ATTRIBUTES (decl)) > + && vnode->ref_list.referring.length ()) > + { > + enum tls_model new_model = decl_default_tls_model (decl); > + gcc_checking_assert (new_model >= decl_tls_model (decl)); > + set_decl_tls_model (decl, new_model); > + } > + } > + } > + > if (dump_file) > { > fprintf (dump_file, "\nMarking local functions:"); > diff --git a/gcc/varasm.cc b/gcc/varasm.cc > index 4db8506b1..de149e82c 100644 > --- a/gcc/varasm.cc > +++ b/gcc/varasm.cc > @@ -6679,6 +6679,36 @@ init_varasm_once (void) > #endif > } > > +/* Determine whether SYMBOL is used in any optimized function. */ > + > +static bool > +have_optimized_refs (struct symtab_node *symbol) > +{ > + struct ipa_ref *ref; > + > + for (int i = 0; symbol->iterate_referring (i, ref); i++) > + { > + cgraph_node *cnode = dyn_cast (ref->referring); > + > + if (cnode && opt_for_fn (cnode->decl, optimize)) > + return true; > + } > + > + return false; > +} > + > +/* Check if promoting general-dynamic TLS access model to local-dynamic is > + desirable for DECL. */ > + > +static bool > +optimize_dyn_tls_for_decl_p (const_tree decl) > +{ > + if (optimize) > + return true; ...this. This is again an access to optimize which in LTO IPA phase is not really meaningful. Can the test be simply removed? If not (but please look at why), I'd suggest to test the overall optimize level only if there is a non-NULL cfun. Otherwise, the patch looks OK to me. Martin > + return symtab->state >= IPA && have_optimized_refs (symtab_node::get (decl)); > +} > + > + > enum tls_model > decl_default_tls_model (const_tree decl) > { > @@ -6696,7 +6726,7 @@ decl_default_tls_model (const_tree decl) > > /* Local dynamic is inefficient when we're not combining the > parts of the address. */ > - else if (optimize && is_local) > + else if (is_local && optimize_dyn_tls_for_decl_p (decl)) > kind = TLS_MODEL_LOCAL_DYNAMIC; > else > kind = TLS_MODEL_GLOBAL_DYNAMIC;