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

[Jan Hubicka <hubicka@kam.mff.cuni.cz>]

> 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