From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by sourceware.org (Postfix) with ESMTPS id 111043858C50 for ; Mon, 2 May 2022 16:43:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 111043858C50 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=ispras.ru Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ispras.ru Received: from [10.10.3.121] (unknown [10.10.3.121]) by mail.ispras.ru (Postfix) with ESMTPS id 94BAB40755C8; Mon, 2 May 2022 16:43:39 +0000 (UTC) Date: Mon, 2 May 2022 19:43:39 +0300 (MSK) From: Alexander Monakov To: Martin Jambor cc: Artem Klimov , Jan Hubicka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] ipa-visibility: Optimize TLS access [PR99619] In-Reply-To: Message-ID: <381c469c-516e-808f-f811-314eddaf2ba9@ispras.ru> References: <20220417185113.25780-1-jakmobius@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, 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, 02 May 2022 16:43:48 -0000 On Mon, 2 May 2022, Martin Jambor wrote: > > Co-Authored-By: Alexander Monakov > > (Minor nit and I don't care too much, but in GCC we traditionally > specify co-authors in the ChangeLog entry beginning by providing more > names, one per line. But perhaps we want to adapt more widely used > practices.) I believe this is the recommended way to specify co-authors now (after Git migration) according to https://gcc.gnu.org/codingconventions.html (patch discussion below) > > --- 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? > 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. > But maybe Honza has some other clever idea. > > Also, please be careful not to unnecessarily commit trailing blank > spaces, the empty lines in your patch are not really empty. Yep, I can take care of whitespace issues. Thank you! Alexander