From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 516AF3858427 for ; Mon, 9 May 2022 16:47:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 516AF3858427 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id D2D82282EB6; Mon, 9 May 2022 18:47:53 +0200 (CEST) Date: Mon, 9 May 2022 18:47:53 +0200 From: Jan Hubicka To: Alexander Monakov Cc: Martin Jambor , Artem Klimov , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] ipa-visibility: Optimize TLS access [PR99619] Message-ID: References: <20220417185113.25780-1-jakmobius@gmail.com> <381c469c-516e-808f-f811-314eddaf2ba9@ispras.ru> <5e8789c-7394-7521-33ae-70375d9a28@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5e8789c-7394-7521-33ae-70375d9a28@ispras.ru> X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 09 May 2022 16:47:57 -0000 > On Mon, 2 May 2022, Alexander Monakov wrote: > > > > --- a/gcc/ipa-visibility.cc > > > > +++ b/gcc/ipa-visibility.cc > > > > @@ -872,6 +872,22 @@ function_and_variable_visibility (bool whole_program) > > > > } > > > > } > > > > } > > > > + FOR_EACH_VARIABLE (vnode) > > > > + { > > > > + tree decl = vnode->decl; > > > > + > > > > + /* Optimize TLS model based on visibility (taking into account > > > > + optimizations done in the preceding loop), unless it was > > > > + specified explicitly. */ > > > > + > > > > + if (DECL_THREAD_LOCAL_P (decl) > > > > + && !lookup_attribute ("tls_model", DECL_ATTRIBUTES (decl))) > > > > + { > > > > + 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); > > > > + } > > > > + } > > > > > > > > > > decl_default_tls_model depends on the global optimize flag, which is > > > almost always problematic in IPA passes. I was able to make your patch > > > ICE using the vis-attr-hidden.c testcase from your patch with: > > > > > > mjambor@virgil:~/gcc/small/tests/tls$ ~/gcc/small/inst/bin/gcc -O2 -fPIC -flto -c vis-attr-hidden.c > > > mjambor@virgil:~/gcc/small/tests/tls$ ~/gcc/small/inst/bin/gcc -fPIC -O0 -shared -flto vis-attr-hidden.o > > > during IPA pass: whole-program > > > lto1: internal compiler error: in function_and_variable_visibility, at ipa-visibility.cc:888 > > [snip] > > > Note the use of LTO, mismatching -O flags and the -shared flag in the > > > link step. > > > > Ah, right. The assert is checking that we don't accidentally downgrade decl's > > TLS access model, e.g. from local-dynamic to global-dynamic, and you've shown > > how to trigger that. I didn't realize this code can run twice, and with > > different 'optimize' levels. > > > > I would suggest to solve this by checking if the new TLS model is stronger, > > i.e. instead of this: > > > > gcc_checking_assert (new_model >= decl_tls_model (decl)); > > set_decl_tls_model (decl, new_model); > > > > do this: > > > > if (new_model >= decl_tls_model (decl)) > > set_decl_tls_model (decl, new_model); > > > > Does this look reasonable? > > On second thought, it might be better to keep the assert, and place the loop > under 'if (optimize)'? The problem is that at IPA level it does not make sense to check optimize flag as it is function specific. (shlib is OK to check it anywhere since it is global.) So I think we really want to run the code only at the WPA time (symtab_state>=IPA_SSA) and we want to see what is optimization flag of those function referring the variable since that is what decided codegen we will produce. Honza > > > > A simple but somewhat lame way to avoid the ICE would be to run your > > > loop over variables only from pass_ipa_function_and_variable_visibility > > > and not from pass_ipa_whole_program_visibility. > > > > > > I am afraid a real solution would involve copying relevant entries from > > > global_options to the symtab node representing the variable when it is > > > created/finalized, properly streaming them for LTO, and modifying > > > decl_default_tls_model to rely on those rather than global_options > > > itself. > > > > If we agree on the solution above, then this will not be necessary, after all > > this transformation looks at optimized whole-program visibility status, > > and so initial optimization level should not be relevant. > > Alexander