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

[Martin Jambor <mjambor@suse.cz>]

Hello,

On Sun, Apr 17 2022, Artem Klimov via Gcc-patches wrote:
> Fix issue PR99619, which asks to optimize TLS access based on
> visibility. The fix is implemented as an IPA optimization, which allows
> to take optimized visibility status into account (as well as avoid
> modifying all language frontends).

I'm afraid I'll have to defer to Honza to approve this, but I have found
an issue with the patch.

>
> 2022-04-17  Artem Klimov  <jakmobius@gmail.com>
>
> gcc/ChangeLog:
> 	PR middle-end/99619
> 	* ipa-visibility.cc (function_and_variable_visibility): Add an
> 	explicit TLS model update after visibility optimisation loops.
>
> gcc/testsuite/ChangeLog:
> 	PR middle-end/99619
> 	* gcc.dg/tls/vis-attr-gd.c: New test.
> 	* gcc.dg/tls/vis-attr-hidden-gd.c: New test.
> 	* gcc.dg/tls/vis-attr-hidden.c: New test.
> 	* gcc.dg/tls/vis-flag-hidden-gd.c: New test.
> 	* gcc.dg/tls/vis-flag-hidden.c: New test.
> 	* gcc.dg/tls/vis-pragma-hidden-gd.c: New test.
> 	* gcc.dg/tls/vis-pragma-hidden.c: New test.
>
> 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.)

> Signed-off-by: Artem Klimov <jakmobius@gmail.com>
> ---

[...]

> diff --git a/gcc/ipa-visibility.cc b/gcc/ipa-visibility.cc
> index e95a0dd252f..ca5b9a95f5e 100644
> --- 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
  0x25f48c0 function_and_variable_visibility
          /home/mjambor/gcc/small/src/gcc/ipa-visibility.cc:888
  0x25f4b33 whole_program_function_and_variable_visibility
          /home/mjambor/gcc/small/src/gcc/ipa-visibility.cc:938
  0x25f4bda execute
          /home/mjambor/gcc/small/src/gcc/ipa-visibility.cc:986
  Please submit a full bug report, with preprocessed source (by using -freport-bug).
  Please include the complete backtrace with any bug report.
  See <https://gcc.gnu.org/bugs/> for instructions.
  lto-wrapper: fatal error: /home/mjambor/gcc/small/inst/bin/gcc returned 1 exit status
  compilation terminated.
  /usr/bin/ld: error: lto-wrapper failed
  collect2: error: ld returned 1 exit status

Note the use of LTO, mismatching -O flags and the -shared flag in the
link step.

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.

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.

Martin