| Message ID | 4F1DE66E.8070701@gmail.com |
|---|---|
| State | New |
| Headers | show |
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~
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.
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
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.
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~
> 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
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.
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;