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 383763858405 for ; Mon, 9 May 2022 16:06:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 383763858405 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 4505640755C2; Mon, 9 May 2022 16:06:01 +0000 (UTC) Date: Mon, 9 May 2022 19:06:01 +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: <381c469c-516e-808f-f811-314eddaf2ba9@ispras.ru> Message-ID: <5e8789c-7394-7521-33ae-70375d9a28@ispras.ru> References: <20220417185113.25780-1-jakmobius@gmail.com> <381c469c-516e-808f-f811-314eddaf2ba9@ispras.ru> 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, 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, 09 May 2022 16:06:11 -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)'? > > 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