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

[Alexander Monakov <amonakov@ispras.ru>]

On Mon, 2 May 2022, Martin Jambor wrote:

> > Co-Authored-By:  Alexander Monakov  <amonakov@gcc.gnu.org>
> 
> (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