llvm dev - Jun 2012 - [LLVMdev] [Patch, RFC] Re: Adding support for explicitly specified TLS models (PR9788)

Hi all,

I would like to hear your thoughts on adding support for extending the
IR to allow for explicitly selecting which model to use for
thread-local storage of a variable.

The motivation is to allow Clang to support the "tls_model" variable
attribute [1], as requested in PR9788.

The idea would be to extend the IR to allow an optional TLS-model
argument to the thread_local attribute, like this:

  @x = thread_local(initialexec) global i32 42

Just as it is illegal to specify thread_local for a target that
doesn't support it, specifying a TLS model that isn't supported by the
target would be illegal. If a model is not specified, the target gets
to choose the appropriate model, as it does today.

Clang would need to know which models are supported by which targets
and give an error (or warn and fall back to the default?) if the user
tries to use an unsupported model, just as it currently gives an error
when trying to use thread-local variables on unsupported targets.

The models in question are described in Section 4 of [2]. As far as I
understand, LLVM supports the following models:

 - X86/ELF: general dynamic, initial exec (but not for PIC code?), local exec
 - ARM/ELF: general dynamic, local exec
 - Mips: general dynamic, local dynamic, initial exec, local exec
 - X86 for Darwin and Windows, and XCore, do their own thing
 - The other targets don't support thread-local storage

If this sounds good, I've got patches coming up.

Thanks,
Hans

[1]
http://gcc.gnu.org/onlinedocs/gcc-4.0.4/gcc/Variable-Attributes.html#index-g_t_0040code_007btls_005fmodel_007d-attribute-1797
[2] http://www.akkadia.org/drepper/tls.pdf

On Wed, Apr 25, 2012 at 12:39:49PM +0100, Hans Wennborg
wrote:
> Just as it is illegal to specify thread_local for a target that
> doesn't support it, specifying a TLS model that isn't supported by
the
> target would be illegal.
For ELF, it doesn't make sense to reject one as it can always be
relaxed. An application program shouldn't have to worry about that, it
is just an implementation detail.

Joerg

On Wed, Apr 25, 2012 at 14:04, Joerg Sonnenberger
<joerg at britannica.bec.de> wrote:
> On Wed, Apr 25, 2012 at 12:39:49PM +0100, Hans Wennborg wrote:
>> Just as it is illegal to specify thread_local for a target that
>> doesn't support it, specifying a TLS model that isn't supported
by the
>> target would be illegal.
>
> For ELF, it doesn't make sense to reject one as it can always be
> relaxed. An application program shouldn't have to worry about that, it
> is just an implementation detail.
I'd be fine with that.

(Clang should still worry about it though, so it can provide a "the
TLS model you selected is not supported by the target, it will fall
back to another one" warning.)

 - Hans

Reviving this thread with a patch!

And some comments inline.

Please take a look and let me know what you think.

 - Hans

On Wed, Apr 25, 2012 at 12:39 PM, Hans Wennborg <hans at chromium.org>
wrote:
> Hi all,
>
> I would like to hear your thoughts on adding support for extending the
> IR to allow for explicitly selecting which model to use for
> thread-local storage of a variable.
>
> The motivation is to allow Clang to support the "tls_model"
variable
> attribute [1], as requested in PR9788.
This enables better performance for some code. For example, TCMalloc
[3] would like to use the initial-exec model, because it's faster.
> The idea would be to extend the IR to allow an optional TLS-model
> argument to the thread_local attribute, like this:
>
>  @x = thread_local(initialexec) global i32 42
>
> Just as it is illegal to specify thread_local for a target that
> doesn't support it, specifying a TLS model that isn't supported by
the
> target would be illegal. If a model is not specified, the target gets
> to choose the appropriate model, as it does today.
As discussed on the original thread, I've changed my mind here. I
don't think there should be any error (or warning) for specifying a
model that isn't supported by a particular target. The target should
just fall back to whatever model it supports.
> The models in question are described in Section 4 of [2]. As far as I
> understand, LLVM supports the following models:
>
>  - X86/ELF: general dynamic, initial exec (but not for PIC code?), local
exec
It now supports all four models.
>  - ARM/ELF: general dynamic, local exec
And also initial exec.
>  - Mips: general dynamic, local dynamic, initial exec, local exec
>  - X86 for Darwin and Windows, and XCore, do their own thing
>  - The other targets don't support thread-local storage
>
> [1]
http://gcc.gnu.org/onlinedocs/gcc-4.0.4/gcc/Variable-Attributes.html#index-g_t_0040code_007btls_005fmodel_007d-attribute-1797
> [2] http://www.akkadia.org/drepper/tls.pdf
[3]
http://code.google.com/searchframe#BGeH2W13jNw/trunk/src/thread_cache.h&l=257
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls_models.diff
Type: application/octet-stream
Size: 36638 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120604/33a322f0/attachment.obj>

On 4 June 2012 10:49, Hans Wennborg <hans at chromium.org>
wrote:
> Reviving this thread with a patch!
>
> And some comments inline.
>
> Please take a look and let me know what you think.
Just a high level comment, why do you need the 4 modes + default?
Can't clang just produce globaldynamic (or the attribute value) and
let llvm optimize it? Since in the end the linker is allowed to relax
tls access too, llvm should be allowed to make the mode more specific
even if the user added an attribute at the C/C++ level.
>  - Hans
Cheers,
Rafael

On Mon, Jun 4, 2012 at 3:49 PM, Hans Wennborg <hans at chromium.org>
wrote:
> Reviving this thread with a patch!
>
> And some comments inline.
>
> Please take a look and let me know what you think.
>
>  - Hans
>
> On Wed, Apr 25, 2012 at 12:39 PM, Hans Wennborg <hans at
chromium.org> wrote:
>> Hi all,
>>
>> I would like to hear your thoughts on adding support for extending the
>> IR to allow for explicitly selecting which model to use for
>> thread-local storage of a variable.
>>
>> The motivation is to allow Clang to support the "tls_model"
variable
>> attribute [1], as requested in PR9788.
>
> This enables better performance for some code. For example, TCMalloc
> [3] would like to use the initial-exec model, because it's faster.
>
>> The idea would be to extend the IR to allow an optional TLS-model
>> argument to the thread_local attribute, like this:
>>
>>  @x = thread_local(initialexec) global i32 42
>>
>> Just as it is illegal to specify thread_local for a target that
>> doesn't support it, specifying a TLS model that isn't supported
by the
>> target would be illegal. If a model is not specified, the target gets
>> to choose the appropriate model, as it does today.
>
> As discussed on the original thread, I've changed my mind here. I
> don't think there should be any error (or warning) for specifying a
> model that isn't supported by a particular target. The target should
> just fall back to whatever model it supports.
>
>> The models in question are described in Section 4 of [2]. As far as I
>> understand, LLVM supports the following models:
>>
>>  - X86/ELF: general dynamic, initial exec (but not for PIC code?),
local exec
>
> It now supports all four models.
>
>>  - ARM/ELF: general dynamic, local exec
>
> And also initial exec.
>
>>  - Mips: general dynamic, local dynamic, initial exec, local exec
>>  - X86 for Darwin and Windows, and XCore, do their own thing
>>  - The other targets don't support thread-local storage
>>
>> [1]
http://gcc.gnu.org/onlinedocs/gcc-4.0.4/gcc/Variable-Attributes.html#index-g_t_0040code_007btls_005fmodel_007d-attribute-1797
>> [2] http://www.akkadia.org/drepper/tls.pdf
>
> [3]
http://code.google.com/searchframe#BGeH2W13jNw/trunk/src/thread_cache.h&l=257
Ping?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls_models.diff
Type: application/octet-stream
Size: 36638 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120608/b2aa1469/attachment.obj>