Re: [PATCH] ipa-visibility: Optimize TLS access [PR99619]

[Alexander Monakov <amonakov@ispras.ru>]

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