Patchwork [trans-mem] Do not instrument thread locals

login
register
mail settings
Submitter Patrick Marlier
Date Jan. 23, 2012, 10:59 p.m.
Message ID <4F1DE66E.8070701@gmail.com>
Download mbox | patch
Permalink /patch/137492/
State New
Headers show

Comments

Patrick Marlier - Jan. 23, 2012, 10:59 p.m.
Hi,

I found that all thread local variables are instrumented with _ITM_W/R* 
calls whereas they should not be shared with other threads. This patch 
takes care of thread locals into requires_barrier and also adds the 
local save/restore for them. This patch also includes a testcase.

I did not fill any PR since I have already a patch for it but tell me if 
I have to.

Tested on x86_64-unknown-linux-gnu, ok for trunk?
Thanks.
--
Patrick Marlier.
ChangeLog
2012-01-23  Patrick Marlier  <patrick.marlier@gmail.com>

	* trans-mem.c (requires_barrier): Do not instrument thread local
	variables and emit save/restore for them.

testsuite/ChangeLog
2012-01-23  Patrick Marlier  <patrick.marlier@gmail.com>

	* gcc.dg/tm/threadlocal-1.c: New test.
Richard Henderson - Jan. 24, 2012, 7:34 a.m.
On 01/24/2012 09:59 AM, Patrick Marlier wrote:
> 2012-01-23  Patrick Marlier  <patrick.marlier@gmail.com>
> 
> 	* trans-mem.c (requires_barrier): Do not instrument thread local
> 	variables and emit save/restore for them.
> 
> testsuite/ChangeLog
> 2012-01-23  Patrick Marlier  <patrick.marlier@gmail.com>
> 
> 	* gcc.dg/tm/threadlocal-1.c: New test.
> 

Ok.


r~
Aldy Hernandez - Jan. 24, 2012, 1:39 p.m.
On 01/24/12 01:34, Richard Henderson wrote:
> On 01/24/2012 09:59 AM, Patrick Marlier wrote:
>> 2012-01-23  Patrick Marlier<patrick.marlier@gmail.com>
>>
>> 	* trans-mem.c (requires_barrier): Do not instrument thread local
>> 	variables and emit save/restore for them.
>>
>> testsuite/ChangeLog
>> 2012-01-23  Patrick Marlier<patrick.marlier@gmail.com>
>>
>> 	* gcc.dg/tm/threadlocal-1.c: New test.
>>
>
> Ok.
>
>
> r~

Thank you Patrick.

Committed.
Andi Kleen - Jan. 25, 2012, 1:32 a.m.
Patrick Marlier <patrick.marlier@gmail.com> writes:

> Hi,
>
> I found that all thread local variables are instrumented with
> _ITM_W/R* calls whereas they should not be shared with other
> threads. This patch takes care of thread locals into requires_barrier
> and also adds the local save/restore for them. This patch also
> includes a testcase.

What happens when the address of the thread local escapes?
It could well be written by another thread then.

I guess you have to check for escapes here.

-Andi
Patrick Marlier - Jan. 25, 2012, 2:30 a.m.
On 01/24/2012 08:32 PM, Andi Kleen wrote:
>> Hi,
>> >
>> >  I found that all thread local variables are instrumented with
>> >  _ITM_W/R* calls whereas they should not be shared with other
>> >  threads. This patch takes care of thread locals into requires_barrier
>> >  and also adds the local save/restore for them. This patch also
>> >  includes a testcase.
> What happens when the address of the thread local escapes?
> It could well be written by another thread then.
>
> I guess you have to check for escapes here.

Thanks to raise the question and I hope I understand your point.

Did you mean something like this?

__thread int myvalue;
void bar()
{
   foo(&myvalue);
   __transaction_atomic {
     myvalue++;
   }
}

where foo shares the pointer to other threads?

 From my point of view, no. When it is a thread local, it should not be 
shared to someone else. If the thread dies, what happens to the thread 
local variable? Should it be discarded completely and this piece of 
memory never reallocated? Even if the programmer take care of this 
situation, does it make sense to share a thread local to other threads?

Anyway, you are probably right but I would prefer let the knowledgeable 
people answer instead of me, the little jedi...
--
Patrick.
Richard Henderson - Jan. 25, 2012, 8:16 p.m.
On 01/25/2012 01:30 PM, Patrick Marlier wrote:
> From my point of view, no. When it is a thread local, it should not
> be shared to someone else. If the thread dies, what happens to the
> thread local variable? Should it be discarded completely and this
> piece of memory never reallocated? Even if the programmer take care
> of this situation, does it make sense to share a thread local to
> other threads?

No, Andi has a point.  It's no more invalid than sharing a variable
off the local stack with another thread.  All that's required is that
the foreign thread not keep the pointer permanently; that the use of
the tls variable end before the thread ends.

And it's entirely likely that I'd thought of exactly that two years
ago when the DECL_THREAD_LOCAL test was omitted from that bit of code,
but I failed to add a comment.

I guess this patch needs to be reverted...


r~
Andi Kleen - Jan. 25, 2012, 11:39 p.m.
> And it's entirely likely that I'd thought of exactly that two years
> ago when the DECL_THREAD_LOCAL test was omitted from that bit of code,
> but I failed to add a comment.
> 
> I guess this patch needs to be reverted...

It may be still a valid optimization, but only if you know there
is no escapes of a static thread variable. But I didn't think
this is known at this point.

I suspect this would require adding another pass for this later in the 
optimization pipeline.

-Andi
Aldy Hernandez - Jan. 26, 2012, 2:10 p.m.
On 01/25/12 14:16, Richard Henderson wrote:
> On 01/25/2012 01:30 PM, Patrick Marlier wrote:
>>  From my point of view, no. When it is a thread local, it should not
>> be shared to someone else. If the thread dies, what happens to the
>> thread local variable? Should it be discarded completely and this
>> piece of memory never reallocated? Even if the programmer take care
>> of this situation, does it make sense to share a thread local to
>> other threads?
>
> No, Andi has a point.  It's no more invalid than sharing a variable
> off the local stack with another thread.  All that's required is that
> the foreign thread not keep the pointer permanently; that the use of
> the tls variable end before the thread ends.
>
> And it's entirely likely that I'd thought of exactly that two years
> ago when the DECL_THREAD_LOCAL test was omitted from that bit of code,
> but I failed to add a comment.
>
> I guess this patch needs to be reverted...

Will do so.

Patch

Index: testsuite/gcc.dg/tm/threadlocal-1.c
===================================================================
--- testsuite/gcc.dg/tm/threadlocal-1.c	(revision 0)
+++ testsuite/gcc.dg/tm/threadlocal-1.c	(revision 0)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O -fdump-tree-tmedge" } */
+__thread int notshared = 0;
+int shared = 0;
+
+int main()
+{
+  __transaction_atomic
+    {
+      notshared++;
+      shared++;
+    }
+  return notshared + shared;
+}
+/* { dg-final { scan-tree-dump-times "tm_save.\[0-9_\]+ = notshared" 1 "tmedge" } } */
+/* { dg-final { scan-tree-dump-times "notshared = tm_save" 1 "tmedge" } } */
+/* { dg-final { cleanup-tree-dump "tmedge" } } */
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 183448)
+++ trans-mem.c	(working copy)
@@ -1,5 +1,5 @@ 
 /* Passes for transactional memory support.
-   Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+   Copyright (C) 2008, 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
 
    This file is part of GCC.
 
@@ -1488,7 +1488,18 @@  requires_barrier (basic_block entry_block, tree x,
 	}
 
       if (is_global_var (x))
-	return !TREE_READONLY (x);
+	{
+	  if (DECL_THREAD_LOCAL_P (x))
+	    goto thread_local;
+	  if (DECL_HAS_VALUE_EXPR_P (x))
+	    {
+	      tree value = get_base_address (DECL_VALUE_EXPR (x));
+
+	      if (value && DECL_P (value) && DECL_THREAD_LOCAL_P (value))
+		goto thread_local;
+	    }
+	  return !TREE_READONLY (x);
+	}
       if (/* FIXME: This condition should actually go below in the
 	     tm_log_add() call, however is_call_clobbered() depends on
 	     aliasing info which is not available during
@@ -1498,17 +1509,14 @@  requires_barrier (basic_block entry_block, tree x,
 	     lower_sequence_tm altogether.  */
 	  needs_to_live_in_memory (x))
 	return true;
-      else
-	{
-	  /* For local memory that doesn't escape (aka thread private
-	     memory), we can either save the value at the beginning of
-	     the transaction and restore on restart, or call a tm
-	     function to dynamically save and restore on restart
-	     (ITM_L*).  */
-	  if (stmt)
-	    tm_log_add (entry_block, orig, stmt);
-	  return false;
-	}
+    thread_local:
+      /* For local memory that doesn't escape (aka thread private memory), 
+	 we can either save the value at the beginning of the transaction and
+	 restore on restart, or call a tm function to dynamically save and
+	 restore on restart (ITM_L*). */
+      if (stmt)
+	tm_log_add (entry_block, orig, stmt);
+      return false;
 
     default:
       return false;