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

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

[Artem Klimov]

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).

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>
Signed-off-by: Artem Klimov <jakmobius@gmail.com>
---
 gcc/ipa-visibility.cc                           | 16 ++++++++++++++++
 gcc/testsuite/gcc.dg/tls/vis-attr-gd.c          | 10 ++++++++++
 gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c   | 11 +++++++++++
 gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c      | 10 ++++++++++
 gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c   | 11 +++++++++++
 gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c      | 10 ++++++++++
 gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c | 15 +++++++++++++++
 gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c    | 14 ++++++++++++++
 8 files changed, 97 insertions(+)
 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 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);
+        }
+    }
 
   if (dump_file)
     {
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
new file mode 100644
index 00000000000..473c7846f74
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
new file mode 100644
index 00000000000..8f592052361
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((visibility("hidden")))
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
new file mode 100644
index 00000000000..2da1bc3fa42
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
+
+//tls_model should be local-dynamic due to visibility("hidden")
+__attribute__((visibility("hidden")))
+__thread int x;
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "visibility" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
new file mode 100644
index 00000000000..de01ef31776
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility -fvisibility=hidden" } */
+
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
new file mode 100644
index 00000000000..3701eaafd62
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility -fvisibility=hidden" } */
+
+
+// tls_model should be local-dynamic due to -fvisibility=hidden
+__thread int x;
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "visibility" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
new file mode 100644
index 00000000000..39cc4bed17d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
+
+
+#pragma GCC visibility push(hidden)
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+#pragma GCC visibility pop
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
new file mode 100644
index 00000000000..1d6b9b144b5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
+
+
+#pragma GCC visibility push(hidden)
+
+// tls_model should be local-dynamic due to a pragma
+__thread int x;
+
+#pragma GCC visibility pop
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "visibility" } } */
-- 
2.25.1

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

[Alexander Monakov]

Hi,

I'd like to ping this patch on behalf of Artem.

Alexander

On Sun, 17 Apr 2022, Artem Klimov 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).
> 
> 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>
> Signed-off-by: Artem Klimov <jakmobius@gmail.com>
> ---
>  gcc/ipa-visibility.cc                           | 16 ++++++++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-attr-gd.c          | 10 ++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c   | 11 +++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c      | 10 ++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c   | 11 +++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c      | 10 ++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c | 15 +++++++++++++++
>  gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c    | 14 ++++++++++++++
>  8 files changed, 97 insertions(+)
>  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 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);
> +        }
> +    }
>  
>    if (dump_file)
>      {
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> new file mode 100644
> index 00000000000..473c7846f74
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> new file mode 100644
> index 00000000000..8f592052361
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((visibility("hidden")))
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> new file mode 100644
> index 00000000000..2da1bc3fa42
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
> +
> +//tls_model should be local-dynamic due to visibility("hidden")
> +__attribute__((visibility("hidden")))
> +__thread int x;
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "visibility" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> new file mode 100644
> index 00000000000..de01ef31776
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility -fvisibility=hidden" } */
> +
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> new file mode 100644
> index 00000000000..3701eaafd62
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility -fvisibility=hidden" } */
> +
> +
> +// tls_model should be local-dynamic due to -fvisibility=hidden
> +__thread int x;
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "visibility" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> new file mode 100644
> index 00000000000..39cc4bed17d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
> +
> +
> +#pragma GCC visibility push(hidden)
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +#pragma GCC visibility pop
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "visibility" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> new file mode 100644
> index 00000000000..1d6b9b144b5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-visibility" } */
> +
> +
> +#pragma GCC visibility push(hidden)
> +
> +// tls_model should be local-dynamic due to a pragma
> +__thread int x;
> +
> +#pragma GCC visibility pop
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "visibility" } } */
> 

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

[Martin Jambor]

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

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

[Alexander Monakov]

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

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

[Alexander Monakov]

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

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

[Jan Hubicka]

> 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

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

[Alexander Monakov]

On Mon, 9 May 2022, Jan Hubicka wrote:

> > 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.

I'm not sure about the latter. Are you suggesting we give up on upgrading
general-dynamic to local-dynamic if in a mixed-O scenario there is at least
one -O0 function referring to the variable? Why? That function will end up
even more deoptimized if we do that!

Alexander

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

[Alexander Monakov]

On Mon, 9 May 2022, Jan Hubicka wrote:

> > 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.

Perhaps I misunderstood the issue. Are you saying that there might be no -O
option on lto1 command line, because lto1 is supposed to take optimization
level from function summaries, but during pass_ipa_whole_program_visibility
there's no "current function" so 'optimize' is at its default value (zero)?

And the solution is to iterate over referring functions to see if at least
one of them satisfies 'opt_for_fn (decl, optimize) > 0'?

Alexander

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

[Alexander Monakov]

On Mon, 16 May 2022, Alexander Monakov wrote:

> On Mon, 9 May 2022, Jan Hubicka wrote:
> 
> > > 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.
> 
> Perhaps I misunderstood the issue. Are you saying that there might be no -O
> option on lto1 command line, because lto1 is supposed to take optimization
> level from function summaries, but during pass_ipa_whole_program_visibility
> there's no "current function" so 'optimize' is at its default value (zero)?
> 
> And the solution is to iterate over referring functions to see if at least
> one of them satisfies 'opt_for_fn (decl, optimize) > 0'?

Do you want to see a patch implementing the above solution?

Alexander

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

[Jan Hubicka]

> On Mon, 16 May 2022, Alexander Monakov wrote:
> 
> > On Mon, 9 May 2022, Jan Hubicka wrote:
> > 
> > > > 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.
> > 
> > Perhaps I misunderstood the issue. Are you saying that there might be no -O
> > option on lto1 command line, because lto1 is supposed to take optimization
> > level from function summaries, but during pass_ipa_whole_program_visibility
> > there's no "current function" so 'optimize' is at its default value (zero)?
> > 
> > And the solution is to iterate over referring functions to see if at least
> > one of them satisfies 'opt_for_fn (decl, optimize) > 0'?
> 
> Do you want to see a patch implementing the above solution?
Yes, I think it is correct solution here...

Thanks,
Honza
> 
> Alexander

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

[Alexander Monakov]

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;
+  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;
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
new file mode 100644
index 000000000..89a248a80
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
new file mode 100644
index 000000000..e32565588
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((visibility("hidden")))
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
new file mode 100644
index 000000000..0d43fc565
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+//tls_model should be local-dynamic due to visibility("hidden")
+__attribute__((visibility("hidden")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
new file mode 100644
index 000000000..cad41e0c8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
+
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
new file mode 100644
index 000000000..a15df092d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
+
+
+// tls_model should be local-dynamic due to -fvisibility=hidden
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
new file mode 100644
index 000000000..3b3598134
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+
+#pragma GCC visibility push(hidden)
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+#pragma GCC visibility pop
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
new file mode 100644
index 000000000..1be976442
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+
+#pragma GCC visibility push(hidden)
+
+// tls_model should be local-dynamic due to a pragma
+__thread int x;
+
+#pragma GCC visibility pop
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
-- 
2.35.1

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

[Alexander Monakov]

Ping.

On Thu, 7 Jul 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;
> +  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;
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> new file mode 100644
> index 000000000..89a248a80
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> new file mode 100644
> index 000000000..e32565588
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((visibility("hidden")))
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> new file mode 100644
> index 000000000..0d43fc565
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +//tls_model should be local-dynamic due to visibility("hidden")
> +__attribute__((visibility("hidden")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> new file mode 100644
> index 000000000..cad41e0c8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> +
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> new file mode 100644
> index 000000000..a15df092d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> +
> +
> +// tls_model should be local-dynamic due to -fvisibility=hidden
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> new file mode 100644
> index 000000000..3b3598134
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +
> +#pragma GCC visibility push(hidden)
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +#pragma GCC visibility pop
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> new file mode 100644
> index 000000000..1be976442
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +
> +#pragma GCC visibility push(hidden)
> +
> +// tls_model should be local-dynamic due to a pragma
> +__thread int x;
> +
> +#pragma GCC visibility pop
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> 

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

[Alexander Monakov]

Ping^2.

On Wed, 20 Jul 2022, Alexander Monakov wrote:

> 
> Ping.
> 
> On Thu, 7 Jul 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;
> > +  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;
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> > new file mode 100644
> > index 000000000..89a248a80
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > +
> > +// tls_model should be global-dynamic due to explicitly specified attribute
> > +__attribute__((tls_model("global-dynamic")))
> > +__thread int x;
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> > new file mode 100644
> > index 000000000..e32565588
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> > @@ -0,0 +1,13 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > +
> > +// tls_model should be global-dynamic due to explicitly specified attribute
> > +__attribute__((visibility("hidden")))
> > +__attribute__((tls_model("global-dynamic")))
> > +__thread int x;
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> > new file mode 100644
> > index 000000000..0d43fc565
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > +
> > +//tls_model should be local-dynamic due to visibility("hidden")
> > +__attribute__((visibility("hidden")))
> > +__thread int x;
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> > new file mode 100644
> > index 000000000..cad41e0c8
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> > @@ -0,0 +1,13 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> > +
> > +
> > +// tls_model should be global-dynamic due to explicitly specified attribute
> > +__attribute__((tls_model("global-dynamic")))
> > +__thread int x;
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> > new file mode 100644
> > index 000000000..a15df092d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> > +
> > +
> > +// tls_model should be local-dynamic due to -fvisibility=hidden
> > +__thread int x;
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> > new file mode 100644
> > index 000000000..3b3598134
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> > @@ -0,0 +1,17 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > +
> > +
> > +#pragma GCC visibility push(hidden)
> > +
> > +// tls_model should be global-dynamic due to explicitly specified attribute
> > +__attribute__((tls_model("global-dynamic")))
> > +__thread int x;
> > +
> > +#pragma GCC visibility pop
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> > new file mode 100644
> > index 000000000..1be976442
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> > @@ -0,0 +1,16 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target fpic } */
> > +/* { dg-require-effective-target tls } */
> > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > +
> > +
> > +#pragma GCC visibility push(hidden)
> > +
> > +// tls_model should be local-dynamic due to a pragma
> > +__thread int x;
> > +
> > +#pragma GCC visibility pop
> > +
> > +void reference() { x++; }
> > +
> > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> > 
> 

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

[Alexander Monakov]

Ping^3.

On Fri, 5 Aug 2022, Alexander Monakov wrote:

> Ping^2.
> 
> On Wed, 20 Jul 2022, Alexander Monakov wrote:
> 
> > 
> > Ping.
> > 
> > On Thu, 7 Jul 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;
> > > +  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;
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> > > new file mode 100644
> > > index 000000000..89a248a80
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> > > @@ -0,0 +1,12 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > > +
> > > +// tls_model should be global-dynamic due to explicitly specified attribute
> > > +__attribute__((tls_model("global-dynamic")))
> > > +__thread int x;
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> > > new file mode 100644
> > > index 000000000..e32565588
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> > > @@ -0,0 +1,13 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > > +
> > > +// tls_model should be global-dynamic due to explicitly specified attribute
> > > +__attribute__((visibility("hidden")))
> > > +__attribute__((tls_model("global-dynamic")))
> > > +__thread int x;
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> > > new file mode 100644
> > > index 000000000..0d43fc565
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> > > @@ -0,0 +1,12 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > > +
> > > +//tls_model should be local-dynamic due to visibility("hidden")
> > > +__attribute__((visibility("hidden")))
> > > +__thread int x;
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> > > new file mode 100644
> > > index 000000000..cad41e0c8
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> > > @@ -0,0 +1,13 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> > > +
> > > +
> > > +// tls_model should be global-dynamic due to explicitly specified attribute
> > > +__attribute__((tls_model("global-dynamic")))
> > > +__thread int x;
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> > > new file mode 100644
> > > index 000000000..a15df092d
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> > > @@ -0,0 +1,12 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> > > +
> > > +
> > > +// tls_model should be local-dynamic due to -fvisibility=hidden
> > > +__thread int x;
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> > > new file mode 100644
> > > index 000000000..3b3598134
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> > > @@ -0,0 +1,17 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > > +
> > > +
> > > +#pragma GCC visibility push(hidden)
> > > +
> > > +// tls_model should be global-dynamic due to explicitly specified attribute
> > > +__attribute__((tls_model("global-dynamic")))
> > > +__thread int x;
> > > +
> > > +#pragma GCC visibility pop
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> > > diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> > > new file mode 100644
> > > index 000000000..1be976442
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> > > @@ -0,0 +1,16 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-require-effective-target fpic } */
> > > +/* { dg-require-effective-target tls } */
> > > +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> > > +
> > > +
> > > +#pragma GCC visibility push(hidden)
> > > +
> > > +// tls_model should be local-dynamic due to a pragma
> > > +__thread int x;
> > > +
> > > +#pragma GCC visibility pop
> > > +
> > > +void reference() { x++; }
> > > +
> > > +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> > > 
> > 
> 

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

[Martin Jambor]

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;

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

[Alexander Monakov]

On Fri, 26 Aug 2022, Martin Jambor wrote:

> > +/* 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.

I'd prefer to keep it. This code is also called from the front-ends when
assigning initial TLS access model (during parsing, at the point where
visibility attributes, if present, have not been processed yet). There
we don't have IPA structures, but 'optimize' is set up.

I also want to avoid iterating over referring functions in non-LTO mode
where trusting 'optimize' should be fine during this IPA pass I think?

Alexander

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

[Martin Jambor]

On Fri, Aug 26 2022, Alexander Monakov wrote:
> On Fri, 26 Aug 2022, Martin Jambor wrote:
>
>> > +/* 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.
>
> I'd prefer to keep it. This code is also called from the front-ends when
> assigning initial TLS access model (during parsing, at the point where
> visibility attributes, if present, have not been processed yet). There
> we don't have IPA structures, but 'optimize' is set up.
>
> I also want to avoid iterating over referring functions in non-LTO mode
> where trusting 'optimize' should be fine during this IPA pass I think?

There is still the optimize attribute so in fact no, even in non-LTO
mode if there is no current function, you cannot trust the "global"
"optimize" thing.

Ideally we would assert that no "analysis" phase of an IPA pass reads
the global optimization flags, so please don't add new places where we
do.

You can either add a parameter to decl_default_tls_model to tell it
what context it is called from and IMHO it would also be acceptable to
check whether we have a non-NULL cfun and decide based on that (but here
I only hope it is not something others might object to).

Martin

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

[Alexander Monakov]

On Tue, 30 Aug 2022, Martin Jambor wrote:

> There is still the optimize attribute so in fact no, even in non-LTO
> mode if there is no current function, you cannot trust the "global"
> "optimize" thing.
> 
> Ideally we would assert that no "analysis" phase of an IPA pass reads
> the global optimization flags, so please don't add new places where we
> do.
> 
> You can either add a parameter to decl_default_tls_model to tell it
> what context it is called from and IMHO it would also be acceptable to
> check whether we have a non-NULL cfun and decide based on that (but here
> I only hope it is not something others might object to).

I see, thank you for explaining the issue, and sorry if I was a bit stubborn.

Does the attached patch (incremental change below) look better? It no longer
has the 'shortcut' where iterating over referrers is avoided for the common
case of plain 'gcc -O2' and no 'optimize' attributes, but fortunately TLS
variables are not so numerous to make chasing that worthwhile.

--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -6703,8 +6703,8 @@ have_optimized_refs (struct symtab_node *symbol)
 static bool
 optimize_dyn_tls_for_decl_p (const_tree decl)
 {
-  if (optimize)
-    return true;
+  if (cfun)
+    return optimize;
   return symtab->state >= IPA && have_optimized_refs (symtab_node::get (decl));
 }

Alexander

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

[Alexander Monakov]

> I see, thank you for explaining the issue, and sorry if I was a bit stubborn.
> 
> Does the attached patch (incremental change below) look better? It no longer
> has the 'shortcut' where iterating over referrers is avoided for the common
> case of plain 'gcc -O2' and no 'optimize' attributes, but fortunately TLS
> variables are not so numerous to make chasing that worthwhile.

... and of course I forgot to add the attachment.  Revised patch below.

---8<---

From b245015ec465604799aef60b224b1e1e264d4cb8 Mon Sep 17 00:00:00 2001
From: Artem Klimov <jakmobius@gmail.com>
Date: Wed, 6 Jul 2022 17:02:01 +0300
Subject: [PATCH] ipa-visibility: Optimize TLS access [PR99619]

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>
---
 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..ffc559431 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 (cfun)
+    return optimize;
+  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;
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
new file mode 100644
index 000000000..89a248a80
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
new file mode 100644
index 000000000..e32565588
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((visibility("hidden")))
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
new file mode 100644
index 000000000..0d43fc565
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+//tls_model should be local-dynamic due to visibility("hidden")
+__attribute__((visibility("hidden")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
new file mode 100644
index 000000000..cad41e0c8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
+
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
new file mode 100644
index 000000000..a15df092d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
+
+
+// tls_model should be local-dynamic due to -fvisibility=hidden
+__thread int x;
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
new file mode 100644
index 000000000..3b3598134
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+
+#pragma GCC visibility push(hidden)
+
+// tls_model should be global-dynamic due to explicitly specified attribute
+__attribute__((tls_model("global-dynamic")))
+__thread int x;
+
+#pragma GCC visibility pop
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
new file mode 100644
index 000000000..1be976442
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target fpic } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
+
+
+#pragma GCC visibility push(hidden)
+
+// tls_model should be local-dynamic due to a pragma
+__thread int x;
+
+#pragma GCC visibility pop
+
+void reference() { x++; }
+
+/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
-- 
2.35.1

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

[Martin Jambor]

Hi,

On Tue, Aug 30 2022, Alexander Monakov wrote:
>> I see, thank you for explaining the issue, and sorry if I was a bit stubborn.
>> 
>> Does the attached patch (incremental change below) look better? It no longer
>> has the 'shortcut' where iterating over referrers is avoided for the common
>> case of plain 'gcc -O2' and no 'optimize' attributes, but fortunately TLS
>> variables are not so numerous to make chasing that worthwhile.
>
> ... and of course I forgot to add the attachment.  Revised patch below.

I hope I am not overstepping my IPA/cgraph maintainer authority here but
I believe the patch is OK for master (assuming it passes bootstrap and
testing).

Thanks,

Martin


>
> ---8<---
>
> From b245015ec465604799aef60b224b1e1e264d4cb8 Mon Sep 17 00:00:00 2001
> From: Artem Klimov <jakmobius@gmail.com>
> Date: Wed, 6 Jul 2022 17:02:01 +0300
> Subject: [PATCH] ipa-visibility: Optimize TLS access [PR99619]
>
> 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>
> ---
>  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..ffc559431 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 (cfun)
> +    return optimize;
> +  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;
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> new file mode 100644
> index 000000000..89a248a80
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-gd.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> new file mode 100644
> index 000000000..e32565588
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden-gd.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((visibility("hidden")))
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> new file mode 100644
> index 000000000..0d43fc565
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-attr-hidden.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +//tls_model should be local-dynamic due to visibility("hidden")
> +__attribute__((visibility("hidden")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> new file mode 100644
> index 000000000..cad41e0c8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden-gd.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> +
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> new file mode 100644
> index 000000000..a15df092d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-flag-hidden.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program -fvisibility=hidden" } */
> +
> +
> +// tls_model should be local-dynamic due to -fvisibility=hidden
> +__thread int x;
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> new file mode 100644
> index 000000000..3b3598134
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden-gd.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +
> +#pragma GCC visibility push(hidden)
> +
> +// tls_model should be global-dynamic due to explicitly specified attribute
> +__attribute__((tls_model("global-dynamic")))
> +__thread int x;
> +
> +#pragma GCC visibility pop
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-global-dynamic" "whole-program" } } */
> diff --git a/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> new file mode 100644
> index 000000000..1be976442
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tls/vis-pragma-hidden.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target fpic } */
> +/* { dg-require-effective-target tls } */
> +/* { dg-options "-O2 -fPIC -fdump-ipa-whole-program" } */
> +
> +
> +#pragma GCC visibility push(hidden)
> +
> +// tls_model should be local-dynamic due to a pragma
> +__thread int x;
> +
> +#pragma GCC visibility pop
> +
> +void reference() { x++; }
> +
> +/* { dg-final { scan-ipa-dump "Varpool flags: tls-local-dynamic" "whole-program" } } */
> -- 
> 2.35.1

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

[Martin Liška]

On 5/2/22 18:04, Martin Jambor wrote:
> (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.)

Hello.

Using Co-Authored-By is fine and actually preferred over the traditional listing
of multiple authors. Moreover, mentioning date and author in a changelog entry
is not necessary, both can be taken from git commit by our changelog hooks.

Cheers,
Martin

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

[Jan Hubicka]

> > @@ -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.

Also note that visibility pass is run twice (once at compile time before
early optimizations and then again at LTO). Since LTO linking may
promote public symbols to local/hidden, perhaps we want to do this only
second time the pass is executed?
> 
> 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.

Hmm, I think it is similar to the semantic_interposition flag added
recently (and causing some interesting issues).

What is the reson we avoid using LOCAL_DYNAMIC with !optimize while we
are happy to use LOCAL_EXEC with !optimize !flag_shlib?

Honza
> 
> Also, please be careful not to unnecessarily commit trailing blank
> spaces, the empty lines in your patch are not really empty.
> 
> Martin

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

[Alexander Monakov]

On Thu, 5 May 2022, Jan Hubicka wrote:

> Also note that visibility pass is run twice (once at compile time before
> early optimizations and then again at LTO). Since LTO linking may
> promote public symbols to local/hidden, perhaps we want to do this only
> second time the pass is executed?

The first pass appears to be redundant and could be avoided, yes.

> What is the reson we avoid using LOCAL_DYNAMIC with !optimize while we
> are happy to use LOCAL_EXEC with !optimize !flag_shlib?

It follows from how local-dynamic model is defined: we call __tls_get_addr
with an argument that identifies the current DSO (not the individual
thread-local variable), and then compute the address of the variable with
a simple addition, so when there are two or more TLS variables, we can
call __tls_get_addr just once (but at -O0 we will end up with redundant
calls).

There's no such concern for local-exec vs initial-exec promotion.

Alexander

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

[Jan Hubicka]

> On Thu, 5 May 2022, Jan Hubicka wrote:
> 
> > Also note that visibility pass is run twice (once at compile time before
> > early optimizations and then again at LTO). Since LTO linking may
> > promote public symbols to local/hidden, perhaps we want to do this only
> > second time the pass is executed?
> 
> The first pass appears to be redundant and could be avoided, yes.
> 
> > What is the reson we avoid using LOCAL_DYNAMIC with !optimize while we
> > are happy to use LOCAL_EXEC with !optimize !flag_shlib?
> 
> It follows from how local-dynamic model is defined: we call __tls_get_addr
> with an argument that identifies the current DSO (not the individual
> thread-local variable), and then compute the address of the variable with
> a simple addition, so when there are two or more TLS variables, we can
> call __tls_get_addr just once (but at -O0 we will end up with redundant
> calls).

Thanks for explanation.
So this is something that really depends on optimization flags of the
function referring the variable rather than on optimization flags of the
variable itself and only makes difference if there is -O0 function that
contains more than one reference to a TLS var?

I guess then a correct answer would be to search for such references.
What happens when there are multiple object files with a hidden TLS var
where some gts LOCAL_DYNAMIC and others GLOBAL_DYNAMIC? (Which may
happen when linking together object files compiled with different
versions of compiler if we go ahead with this patch on hidden symbols).

Honza
> 
> There's no such concern for local-exec vs initial-exec promotion.
> 
> Alexander

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

[Alexander Monakov]

On Thu, 5 May 2022, Jan Hubicka wrote:

> > It follows from how local-dynamic model is defined: we call __tls_get_addr
> > with an argument that identifies the current DSO (not the individual
> > thread-local variable), and then compute the address of the variable with
> > a simple addition, so when there are two or more TLS variables, we can
> > call __tls_get_addr just once (but at -O0 we will end up with redundant
> > calls).
> 
> Thanks for explanation.
> So this is something that really depends on optimization flags of the
> function referring the variable rather than on optimization flags of the
> variable itself and only makes difference if there is -O0 function that
> contains more than one reference to a TLS var?

Well, for an -O0 function it doesn't matter how many different TLS variables
it is referencing. The interesting case is an -O2 function referencing one
local-dynamic TLS variable.

> I guess then a correct answer would be to search for such references.

Presumably at RTL generation time, i.e. let the middle end discover the
most specific TLS access model, and then selectively downgrade local-dynamic
to global-dynamic when lowering an -O0 function.

> What happens when there are multiple object files with a hidden TLS var
> where some gts LOCAL_DYNAMIC and others GLOBAL_DYNAMIC? (Which may
> happen when linking together object files compiled with different
> versions of compiler if we go ahead with this patch on hidden symbols).

They have different relocations, so there's an increase in number of GOT
entries, but otherwise I don't think there's any problem.

Alexander