[updated] Make emulated TLS lto-friendly.

This is an update of a patch originally posted and discussed in this  
thread:
http://gcc.gnu.org/ml/gcc-patches/2010-06/threads.html#00995

I guess it needs a global approver - so cc-ing Diego who, IIRC,  
approved the previous EMUTLS changes.

changes from the previous thread.
(a) emutls.c is no longer modified in any way.
(b) I have taken on board the comments about not rearranging code.
(c) the cases where we should not have emutls variables are now asserts.

tested on {i686,powerpc*}-apple-darwin{8,9},  x86_64-apple-darwin10 -  
EMUTLS target without aliases.
    {i686,x86_64}unknown-linux-gnu - native TLS targets (i.e. for a  
null response)
   cris-elf(sim)  - EMUTLS target with aliases
  + armel-linux-eabi(compile-only), mipsabi64-elf(sim), s390x(sompile  
only).

The test-suite additions are motivated by:
(a) the fact that initialization of the emults vars was a weakness  
w.r.t. optimization,
(b) to include the test that is used as the configury check for TLS  
compliance.


OK for trunk now? (and eventually 4.5)?

Iain

gcc/Changelog (email addresses omitted)
	
	Iain Sandoe
	Jan Hubika

	PR target/44132
	* expr.c (emutls_var_address): Remove.
	(expand_expr_addr_expr_1): Remove TLS emulation hook.
	(expand_expr_real_1): Ditto.

	* gimplify.c (emutls_var_address): Add proc.
	(gimplify_decl_expr): expand TLS vars.
	(gimplify_var_or_parm_decl): Ditto.
	(omp_notice_variable): Recognize TLS_MODEL_EMULATED.

	* passes.c (rest_of_decl_compilation): Substitute TLS control vars  
for the master.

	* varasm.c (get_emutls_init_templ_addr): Do not reset the caller's  
DECL_INITIAL.
	(emutls_decl): copy DECL_PRESERVE_P instead of setting it, copy
	TREE_ADDRESSABLE.  Create the init template as soon as we see a valid  
initializer.
	 Mark the original var with error_mark_node so that re- 
initializations will be diagnosed
	properly.  Mark the original var as not to be output.
	(emutls_common_1): Do not rely on DECL_COMMON as a decision on  
whether a constructor
	is needed, it is also required for lcomm.   Ensure that the var is  
laid out before the constructor
	is built.
	(emutls_finalize_control_var): Assert on incorrect type, copy USED.
	(asm_output_bss): Assert on DECL_THREAD_LOCAL_P for EMUTLS targets.
	(asm_output_aligned_bss): Likewise.
	(assemble_variable): Remove TLS var. substitution.  Back up  
TREE_ASM_WRITTEN so that
	assemble_variable() has no side-effects for unwritten vars.  Assert  
on DECL_THREAD_LOCAL_P
	for EMUTLS targets.
	(do_assemble_alias): Assert on DECL_THREAD_LOCAL_P for EMUTLS targets.
	(var_decl_for_asm): New.
	(finish_aliases_1): Walk alias pairs substituting tls controls for  
originals.

	* passes.c (rest_of_decl_compilation): Substitute TLS control vars  
for the master.

	* varpool.c (varpool_mark_needed_node): Do not handle TLS  
substitution here.
	(decide_is_variable_needed): Or here.
	(varpool_finalize_decl): Handle TLS substitution.
	Remove early calls to varpool_assemble_pending_decls().
	Check enqueuing of vars after all tests for need are complete.
	(varpool_analyze_pending_decls): Do not record references if the  
initializer is error_mark.

testsuite:
	
	PR target/44132
	* gcc.dg/tls/thr-init-1.c: New.
	* gcc.dg/tls/thr-init-2.c: New.
	* gcc.dg/torture/tls New.
	* gcc.dg/torture/tls/tls-test.c: New.
	* gcc.dg/torture/tls/thr-init-1.c: New.
	* gcc.dg/torture/tls/tls.exp: New.
	* gcc.dg/torture/tls/thr-init-2.c: New.


Index: gcc/testsuite/gcc.dg/tls/thr-init-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tls/thr-init-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tls/thr-init-1.c	(revision 0)
@@ -0,0 +1,8 @@
+/* { dg-require-effective-target tls } */
+/* { dg-do compile } */
+
+static __thread int fstat ;
+static __thread int fstat = 1 ;
+static __thread int fstat ;
+static __thread int fstat = 2; /* { dg-error "redefinition of 'fstat'" } */
+				/* { dg-message "note: previous definition of 'fstat' was here" "" { target *-*-* } 5 } */
Index: gcc/testsuite/gcc.dg/tls/thr-init-2.c
===================================================================
--- gcc/testsuite/gcc.dg/tls/thr-init-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tls/thr-init-2.c	(revision 0)
@@ -0,0 +1,23 @@
+/* { dg-require-effective-target tls } */
+/* { dg-do run } */
+
+extern void abort() ;
+
+static __thread int fstat ;
+static __thread int fstat = 1;
+
+int test_code(int b)
+{
+  fstat += b ;
+  return fstat;
+}
+
+int main (int ac, char *av[])
+{
+  int a = test_code(1);
+  
+  if ((a != 2) || (fstat != 2))
+    abort () ;
+  
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/tls/tls-test.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/tls/tls-test.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/tls/tls-test.c	(revision 0)
@@ -0,0 +1,51 @@
+/* { dg-do run }  */
+/* { dg-require-effective-target tls  }  */
+/* { dg-require-effective-target pthread } */
+
+#include <pthread.h>
+extern int printf (char *,...);
+__thread int a = 5; 
+int *volatile a_in_other_thread = (int *) 12345;
+
+static void *
+thread_func (void *arg)
+{
+  a_in_other_thread = &a;
+  a+=5;
+  *((int *) arg) = a;
+  return (void *)0;
+}
+
+int
+main ()
+{
+  pthread_t thread;
+  void *thread_retval;
+  int *volatile a_in_main_thread;
+  int *volatile again ;
+  int thr_a;
+
+  a_in_main_thread = &a;
+
+  if (pthread_create (&thread, (pthread_attr_t *)0, thread_func, &thr_a))
+    return 0;
+
+  if (pthread_join (thread, &thread_retval))
+    return 0;
+
+  again = &a;
+  if (again != a_in_main_thread)
+    {
+      printf ("FAIL: main thread addy changed from 0x%0x to 0x%0x\n", 
+		a_in_other_thread, again);
+      return 1;
+    }
+
+  if (a != 5 || thr_a != 10 || (a_in_other_thread == a_in_main_thread))
+    {
+      printf ("FAIL: a= %d, thr_a = %d Addr = 0x%0x\n", 
+		a, thr_a, a_in_other_thread);
+      return 1;
+    }
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/tls/thr-init-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/tls/thr-init-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/tls/thr-init-1.c	(revision 0)
@@ -0,0 +1,25 @@
+/* { dg-do run } */
+/* { dg-require-effective-target tls } */
+
+extern int printf (char *,...);
+extern void abort() ;
+
+int test_code(int b)
+{
+static __thread int fstat = 1;
+  fstat += b ;
+  return fstat;
+}
+
+int main (int ac, char *av[])
+{
+  int a = test_code(1);
+  
+  if ( a != 2 )
+    {
+      printf ("a=%d\n", a) ;
+      abort ();
+    }
+  
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/tls/tls.exp
===================================================================
--- gcc/testsuite/gcc.dg/torture/tls/tls.exp	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/tls/tls.exp	(revision 0)
@@ -0,0 +1,36 @@
+#   Copyright (C) 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with GCC; see the file COPYING3.  If not see
+# <http://www.gnu.org/licenses/>.
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Load support procs.
+load_lib gcc-dg.exp
+
+# If a testcase doesn't have special options, use these.
+global DEFAULT_CFLAGS
+if ![info exists DEFAULT_CFLAGS] then {
+    set DEFAULT_CFLAGS " -ansi -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
+        $DEFAULT_CFLAGS
+
+# All done.
+dg-finish
Index: gcc/testsuite/gcc.dg/torture/tls/thr-init-2.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/tls/thr-init-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/tls/thr-init-2.c	(revision 0)
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-require-effective-target tls } */
+
+extern int printf (char *,...);
+extern void abort() ;
+
+static __thread int fstat ;
+static __thread int fstat = 1;
+static __thread int fstat ;
+
+int test_code(int b)
+{
+  fstat += b ;
+  return fstat;
+}
+
+int main (int ac, char *av[])
+{
+  int a = test_code(1);
+  
+  if ( a != 2 || fstat != 2 )
+    {
+    printf ("a=%d fstat=%d\n", a, fstat) ;
+    abort ();
+    }
+  
+  return 0;
+}

On 07/03/2010 07:10 AM, IainS wrote:
> +      return GS_OK ;

s/ ;/;/g

There are at least 2 occurrences.

> +      if ((DECL_TLS_MODEL (decl) >= TLS_MODEL_EMULATED))

Extra ().

Possibly better written as != TLS_MODEL_NONE as well.

> +	  if (value && DECL_P (value) && 
> +	      (DECL_TLS_MODEL (value) >= TLS_MODEL_EMULATED))

> +  if (!DECL_INITIAL (to) && 
> +      DECL_INITIAL (decl) && (DECL_INITIAL (decl) != error_mark_node)) 
> +    if (! DECL_EXTERNAL (to) && ! DECL_COMMON (to))

Coding style -- && should be on the new line.
Extra ().
Merge the unnecessarily nested ifs.

> +  /* ??? It is not enough to check DECL_COMMON because variables might be 
> +     allocated in other uninitialized sections.  However, this might still
> +     not be an adequate test.  */
> +  if (DECL_INITIAL (h->to))

Huh?  Why is DECL_COMMON not sufficient?

> +  /* Refuse to build a zero-sized item, first make sure there's
> +     a valid layout.  */
> +  if (DECL_SIZE (h->base.from) == 0)
> +    layout_decl (h->base.from, 0);

Really?  What is a testcase for which the decl is not layed out?
That really sounds like a bug elsewhere.

> +  gcc_assert (DECL_SIZE (h->base.from) != 0);

I'm pretty sure you can declare zero sized objects as a gcc extension.

$ cat z.c
__thread int x[0] __attribute__((common));
$ cat z.s
	.file	"z.c"
	.tls_common	x,0,4
	.ident	"GCC: (GNU) 4.4.4 20100630 (Red Hat 4.4.4-10)"
	.section	.note.GNU-stack,"",@progbits

So here's a test case that will trigger that abort.

> +  /* We don't emit the userland vars for emulated TLS - they should never
> +     get to here, only the control vars should be emitted.  */
> +  gcc_assert (targetm.have_tls  
> +	      || (TREE_CODE (decl) != VAR_DECL)
> +	      || !DECL_THREAD_LOCAL_P (decl));

Extra ().  I'm sure there are more, but I won't comment again.
This pattern occurs many places.  Pull this out to a function.

> +  /* If we need the var, and it's an emulated TLS entity, that
> +     means we need the control var.  */
> +  if (!targetm.have_tls && DECL_THREAD_LOCAL_P (node->decl))
> +    {
> +      struct varpool_node *cv_node;
> +      cv_node = varpool_node (emutls_decl (node->decl)) ;
> +      varpool_mark_needed_node (cv_node);
> +    }

Do you really need this kind of thing anymore, since you're exposing
the use of the control variable so early?  I would have thought that
varpool.c would no longer need any special-casing for !have_tls.

Finally, as a follow-up, I think the alias oracle needs to be taught what 
__builtin_emutls_get_address does.  Namely, return the address of the
associated TLS variable.  This is the only way I can think of to avoid
the optimization penalty that you're introducing because of exposing the
control variable and the function call at this point.


r~

Hello Richard,
Thanks for the review and sorry about the typographical nits...
I'll re-work the other things and investigate the alias oracle.

one question tho:

On 8 Jul 2010, at 00:22, Richard Henderson wrote:

>> +  /* ??? It is not enough to check DECL_COMMON because variables  
>> might be
>> +     allocated in other uninitialized sections.  However, this  
>> might still
>> +     not be an adequate test.  */
>> +  if (DECL_INITIAL (h->to))
>
> Huh?  Why is DECL_COMMON not sufficient?

int foo (void)
{
   static __thread int a;
   return a;
}

places __emutls_v.a.1700  in .lcomm and therefore needs an  
initializer, but the decl is not marked DECL_COMMON.
do you believe that indicates a bug elsewhere, or is it a reasonable  
explanation?

thanks
Iain

On Thu, Jul 8, 2010 at 1:22 AM, Richard Henderson <rth@redhat.com> wrote:

> Finally, as a follow-up, I think the alias oracle needs to be taught what
> __builtin_emutls_get_address does.  Namely, return the address of the
> associated TLS variable.  This is the only way I can think of to avoid
> the optimization penalty that you're introducing because of exposing the
> control variable and the function call at this point.

While teaching the oracle about __builtin_emutls_get_address will
help disambiguating against emutls accesses it will not be enough
to trigger CSE.  So it would be nice to have a testcase that shows
we can properly CSE emutls accesses in FRE (in theory it shuld
work as __builtin_emutls_get_address is const).

Richard.

>
>
> r~
>

On Thu, Jul 08, 2010 at 09:42:16AM +0100, IainS wrote:
> >Huh?  Why is DECL_COMMON not sufficient?
> 
> int foo (void)
> {
>   static __thread int a;
>   return a;
> }

This testcase should be optimized into just
int foo (void)
{
  return 0;
}
with no TLS vars at all, at least it is with non-emulated TLS.
Please make sure it doesn't regress for emulated TLS with your patch.

	Jakub

On 8 Jul 2010, at 10:34, Jakub Jelinek wrote:

> On Thu, Jul 08, 2010 at 09:42:16AM +0100, IainS wrote:
>>> Huh?  Why is DECL_COMMON not sufficient?
>>
>> int foo (void)
>> {
>>  static __thread int a;
>>  return a;
>> }
>
> This testcase should be optimized into just
> int foo (void)
> {
>  return 0;
> }
> with no TLS vars at all, at least it is with non-emulated TLS.
> Please make sure it doesn't regress for emulated TLS with your patch.

Well, this wasn't the purpose of the example, O0 is sufficient for that.

However, it *does* regress on this test, so I've got more work to do  
there.
Thanks for catching that.

Iain.

On 07/08/2010 01:42 AM, IainS wrote:
>> Huh?  Why is DECL_COMMON not sufficient?
> 
> int foo (void)
> {
>   static __thread int a;
>   return a;
> }
> 
> places __emutls_v.a.1700  in .lcomm and therefore needs an initializer,
> but the decl is not marked DECL_COMMON.
> do you believe that indicates a bug elsewhere, or is it a reasonable
> explanation?

The purpose of __emutls_register_common is to merge COMMON block sizes,
as would normally be done in the linker.  The variable A is not a COMMON
variable (as you note by DECL_COMMON not being set).  No initialization
is needed.

You've simply been confused about .lcomm for zero-initialization of the
control variable and true COMMON block variables.


r~

On 8 Jul 2010, at 17:14, Richard Henderson wrote:

> On 07/08/2010 01:42 AM, IainS wrote:
>>> Huh?  Why is DECL_COMMON not sufficient?
>>
>> int foo (void)
>> {
>>  static __thread int a;
>>  return a;
>> }
>>
>> places __emutls_v.a.1700  in .lcomm and therefore needs an  
>> initializer,
>> but the decl is not marked DECL_COMMON.
>> do you believe that indicates a bug elsewhere, or is it a reasonable
>> explanation?
>
> The purpose of __emutls_register_common is to merge COMMON block  
> sizes,
> as would normally be done in the linker.  The variable A is not a  
> COMMON
> variable (as you note by DECL_COMMON not being set).  No  
> initialization
> is needed.
>
> You've simply been confused about .lcomm for zero-initialization of  
> the
> control variable and true COMMON block variables.

I'm sure it's confusion on my part - but :

__emutls_v.a
has size and align fields which cannot be 0.

so, either it should not be placed into .lcomm (I infer this is  
probably the case from your response), or these fields must be  
initialized by the emutls_register_common hook.

Iain

On 07/08/2010 09:20 AM, IainS wrote:
> I'm sure it's confusion on my part - but :
> 
> __emutls_v.a
> has size and align fields which cannot be 0.
> 
> so, either it should not be placed into .lcomm (I infer this is probably
> the case from your response), or these fields must be initialized by the
> emutls_register_common hook.

Err, that's true.  In this case I'd expect __emutls_v.a to *not*
reside in .lcomm and not be initialized by the runtime.  Certainly
that's what happens with current HEAD.


r~

On 07/08/2010 05:00 AM, IainS wrote:
> On 8 Jul 2010, at 10:34, Jakub Jelinek wrote:
>> with no TLS vars at all, at least it is with non-emulated TLS.
>> Please make sure it doesn't regress for emulated TLS with your patch.
> 
> Well, this wasn't the purpose of the example, O0 is sufficient for that.
> 
> However, it *does* regress on this test, so I've got more work to do there.
> Thanks for catching that.

I very much doubt you'll be able to fix this (minor) regression.

The only reasonable way to prevent that is to *not* lower the TLS
variable so early, so that the optimizers see DECLs where they 
expect to see DECLs.  And that goal appears to be at odds with
making emulated tls LTO friendly.


r~

> On 07/08/2010 05:00 AM, IainS wrote:
> > On 8 Jul 2010, at 10:34, Jakub Jelinek wrote:
> >> with no TLS vars at all, at least it is with non-emulated TLS.
> >> Please make sure it doesn't regress for emulated TLS with your patch.
> > 
> > Well, this wasn't the purpose of the example, O0 is sufficient for that.
> > 
> > However, it *does* regress on this test, so I've got more work to do there.
> > Thanks for catching that.
> 
> I very much doubt you'll be able to fix this (minor) regression.
> 
> The only reasonable way to prevent that is to *not* lower the TLS
> variable so early, so that the optimizers see DECLs where they 
> expect to see DECLs.  And that goal appears to be at odds with
> making emulated tls LTO friendly.

There is option to have emultls lowering pass somewhere at the end of early
optimization queue.  I am not however sure it is worth a special purpose pass.
Not lowering TLS is also leading to code quality problems, for instance inliner
will not take into account htat functions using TLS are rather expensive etc.

Honza
> 
> 
> r~

On 07/08/2010 10:20 AM, Jan Hubicka wrote:
> There is option to have emultls lowering pass somewhere at the end of early
> optimization queue.  I am not however sure it is worth a special purpose pass.

I'm not sure it's worth it either.  I can't think of any interesting early
optimizations that doesn't work equally well with the const function call.

> Not lowering TLS is also leading to code quality problems, for instance inliner
> will not take into account htat functions using TLS are rather expensive etc.

Well, *that* should simply be a matter of adjusting the costs.  Because
we'll never be able to lower real TLS -- the code sequences required for
linker optimization are extremely specific, padding prefixes and all.


r~

On 8 Jul 2010, at 00:22, Richard Henderson wrote:

>
>> +  /* If we need the var, and it's an emulated TLS entity, that
>> +     means we need the control var.  */
>> +  if (!targetm.have_tls && DECL_THREAD_LOCAL_P (node->decl))
>> +    {
>> +      struct varpool_node *cv_node;
>> +      cv_node = varpool_node (emutls_decl (node->decl)) ;
>> +      varpool_mark_needed_node (cv_node);
>> +    }
>
> Do you really need this kind of thing anymore, since you're exposing
> the use of the control variable so early?  I would have thought that
> varpool.c would no longer need any special-casing for !have_tls.

this is my understanding (which might be flawed &| incomplete).

Whilst we are on the parse side - and building varpool & cgraph, the  
relationship is not fully exposed (that happens when gimplication is  
done).

So my reasoning was that the "ghost/proxy" vars should be made to  
track the user-land ones until then.

It would be possible to put all the checks for 'used-ness' and  
finalization in emutls_finish() but it seemed more elegant to do so in  
the places I put it.

I'm happy either way ..

Iain

On 07/08/2010 12:07 PM, IainS wrote:
> 
> On 8 Jul 2010, at 00:22, Richard Henderson wrote:
>> Do you really need this kind of thing anymore, since you're exposing
>> the use of the control variable so early?  I would have thought that
>> varpool.c would no longer need any special-casing for !have_tls.
> 
> this is my understanding (which might be flawed &| incomplete).
> 
> Whilst we are on the parse side - and building varpool & cgraph, the
> relationship is not fully exposed (that happens when gimplication is done).
> 
> So my reasoning was that the "ghost/proxy" vars should be made to track
> the user-land ones until then.

Hmm.  So what you're saying is that there's extra cleanup work that
needs to happen while lowering the representation.  It's not merely
a matter of code substitution during gimplification.

In which case I wonder if it wouldn't be better to do as Honza
suggested and separate all of this out into a new pass_lower_emutls.
Perhaps to be placed just after pass_lower_vector.  That placement
is before pass_build_cgraph_edges, which I believe means you would
not have to fix up the cgraph edges for the new function calls.  All
you'd need to do is transform the tls variable references and fix up
the varpool references.

Honza, thoughts here?


r~