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

[Martin Jambor <mjambor@suse.cz>]

Hi,

sorry for ignoring this for so long, I hope that Honza wold take over.

I think the patch would be good if it did not have....

On Thu, Jul 07 2022, Alexander Monakov via Gcc-patches wrote:
> From: Artem Klimov <jakmobius@gmail.com>
>
> Fix PR99619, which asks to optimize TLS model based on visibility.
> The fix is implemented as an IPA optimization: this allows to take
> optimized visibility status into account (as well as avoid modifying
> all language frontends).
>
> 2022-04-17  Artem Klimov  <jakmobius@gmail.com>
>
> gcc/ChangeLog:
>
> 	* ipa-visibility.cc (function_and_variable_visibility): Promote
> 	TLS access model afer visibility optimizations.
> 	* varasm.cc (have_optimized_refs): New helper.
> 	(optimize_dyn_tls_for_decl_p): New helper. Use it ...
> 	(decl_default_tls_model): ... here in place of 'optimize' check.
>
> gcc/testsuite/ChangeLog:
>
> 	* 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>
> Signed-off-by: Artem Klimov <jakmobius@gmail.com>
> ---
>
> v2: run the new loop in ipa-visibility only in the whole-program IPA pass;
>     in decl_default_tls_model, check if any referring function is optimized
>     when 'optimize == 0' (when running in LTO mode)
>
>
> Note for reviewers: I noticed there's a place which tries to avoid TLS
> promotion, but the comment seems wrong and I could not find a testcase.
> I'd suggest we remove it. The compiler can only promote general-dynamic
> to local-dynamic and initial-exec to local-exec. The comment refers to
> promoting x-dynamic to y-exec, but that cannot happen AFAICT:
> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=8e1ba78f1b8eedd6c65c6f0e6d6d09a801de5d3d
>
>
>  gcc/ipa-visibility.cc                         | 19 +++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-attr-gd.c        | 12 +++++++
>  gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c | 13 ++++++++
>  gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c    | 12 +++++++
>  gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c | 13 ++++++++
>  gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c    | 12 +++++++
>  .../gcc.dg/tls/vis-pragma-hidden-gd.c         | 17 ++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c  | 16 ++++++++++
>  gcc/varasm.cc                                 | 32 ++++++++++++++++++-
>  9 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
>  create mode 100644 gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
>
> diff --git a/gcc/ipa-visibility.cc b/gcc/ipa-visibility.cc
> index 8a27e7bcd..3ed2b7cf6 100644
> --- a/gcc/ipa-visibility.cc
> +++ b/gcc/ipa-visibility.cc
> @@ -873,6 +873,25 @@ function_and_variable_visibility (bool whole_program)
>  	}
>      }
>  
> +  if (symtab->state >= IPA_SSA)
> +    {
> +      FOR_EACH_VARIABLE (vnode)
> +	{
> +	  tree decl = vnode->decl;
> +
> +	  /* Upgrade TLS access model based on optimized visibility status,
> +	     unless it was specified explicitly or no references remain.  */
> +	  if (DECL_THREAD_LOCAL_P (decl)
> +	      && !lookup_attribute ("tls_model", DECL_ATTRIBUTES (decl))
> +	      && vnode->ref_list.referring.length ())
> +	    {
> +	      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);
> +	    }
> +	}
> +    }
> +
>    if (dump_file)
>      {
>        fprintf (dump_file, "\nMarking local functions:");
> diff --git a/gcc/varasm.cc b/gcc/varasm.cc
> index 4db8506b1..de149e82c 100644
> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -6679,6 +6679,36 @@ init_varasm_once (void)
>  #endif
>  }
>  
> +/* Determine whether SYMBOL is used in any optimized function.  */
> +
> +static bool
> +have_optimized_refs (struct symtab_node *symbol)
> +{
> +  struct ipa_ref *ref;
> +
> +  for (int i = 0; symbol->iterate_referring (i, ref); i++)
> +    {
> +      cgraph_node *cnode = dyn_cast <cgraph_node *> (ref->referring);
> +
> +      if (cnode && opt_for_fn (cnode->decl, optimize))
> +	return true;
> +    }
> +
> +  return false;
> +}
> +
> +/* Check if promoting general-dynamic TLS access model to local-dynamic is
> +   desirable for DECL.  */
> +
> +static bool
> +optimize_dyn_tls_for_decl_p (const_tree decl)
> +{
> +  if (optimize)
> +    return true;

...this.  This is again an access to optimize which in LTO IPA phase is
not really meaningful.  Can the test be simply removed?  If not (but
please look at why), I'd suggest to test the overall optimize level only
if there is a non-NULL cfun.

Otherwise, the patch looks OK to me.

Martin

> +  return symtab->state >= IPA && have_optimized_refs (symtab_node::get (decl));
> +}
> +
> +
>  enum tls_model
>  decl_default_tls_model (const_tree decl)
>  {
> @@ -6696,7 +6726,7 @@ decl_default_tls_model (const_tree decl)
>  
>    /* Local dynamic is inefficient when we're not combining the
>       parts of the address.  */
> -  else if (optimize && is_local)
> +  else if (is_local && optimize_dyn_tls_for_decl_p (decl))
>      kind = TLS_MODEL_LOCAL_DYNAMIC;
>    else
>      kind = TLS_MODEL_GLOBAL_DYNAMIC;