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

> Attaching a new patch that has the behaviour we discussed.
>
> The "globaldynamic" and default values have been merged, and LLVM
will
> start off with the user-specified model, but choose a more specific
> one if possible.
>
> Please review.
Awesome, thanks!
I will try to do a more complete review tonight or tomorrow. For now,
just two quick observations

*) This breaks the clang build, it would be nice to for the first
commit to keep the old API. It can be removed as soon as clang is
updated.
*) Please name the most general model GeneralDynamicTLSMode. Elf's
default visibility being called 'default' is already confusing enough
:-)

I was not sure how hard the first item would be, so I just gave it a
try. The resulting patch is attached.
> Thanks,
> Hans
Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: application/octet-stream
Size: 38564 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120620/61fd4a44/attachment.obj>

On Wed, Jun 20, 2012 at 9:29 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> Attaching a new patch that has the behaviour we discussed.
>>
>> The "globaldynamic" and default values have been merged, and
LLVM will
>> start off with the user-specified model, but choose a more specific
>> one if possible.
>>
>> Please review.
>
> Awesome, thanks!
> I will try to do a more complete review tonight or tomorrow.
Great, looking forward to landing this :)
> For now,
> just two quick observations
>
> *) This breaks the clang build, it would be nice to for the first
> commit to keep the old API. It can be removed as soon as clang is
> updated.
I have the clang patch ready (it was reviewed on cfe-commits), so my
plan was just to commit them both at once.

If we don't want to do that, I think we should try to add the new
constructors while keeping the old ones around, and then delete the
old constructors once clang is updated.
> *) Please name the most general model GeneralDynamicTLSMode. Elf's
> default visibility being called 'default' is already confusing
enough
> :-)
I was thinking that calling it GeneralDynamicTLSMode wouldn't make
sense for non-ELF targets. My thinking was that DefaultTLSModel would
mean "use the default model for the target, which for ELF is general
dynamic, and on Darwin is whatever Darwin does, etc.".
> I was not sure how hard the first item would be, so I just gave it a
> try. The resulting patch is attached.
Your patch preserves the current constructors. Is the idea that we
wouldn't change the constructors, and clang would call
setThreadLocalMode() when it creates global variables? I would feel
better if we changed the constructors, to avoid the risk of forgetting
to do stuff like
NewGV->setThreadLocalMode(OldGV->getThreadLocalMode()) when new
variables are created based on old ones.

Thanks,
Hans

> If we don't want to do that, I think we should try to add the new
> constructors while keeping the old ones around, and then delete the
> old constructors once clang is updated.
Yes, this would probably be the best. To make clang build I just
hacked the constructors in the patch I posted, but they should really
just forward to the new ones.
>> *) Please name the most general model GeneralDynamicTLSMode. Elf's
>> default visibility being called 'default' is already confusing
enough
>> :-)
>
> I was thinking that calling it GeneralDynamicTLSMode wouldn't make
> sense for non-ELF targets. My thinking was that DefaultTLSModel would
> mean "use the default model for the target, which for ELF is general
> dynamic, and on Darwin is whatever Darwin does, etc.".
This is a good point, but it applies to the other names too, right?
For Darwin, all 4 models map to the one that Darwin has. The two
closest examples to this in llvm are linkage and visibility. For
linkage we have our own names and a superset of any object format. For
visibility we use the ELF visibility names and other targets map it to
what they have (macho has only default and hidden I think).

It should at least be self consistent I think. I am ok with 4 llvm
specific names or using the 4 names used by the tlf.pdf document. Just
make sure none of them is named 'default' :-)
> Your patch preserves the current constructors. Is the idea that we
> wouldn't change the constructors, and clang would call
> setThreadLocalMode() when it creates global variables? I would feel
> better if we changed the constructors, to avoid the risk of forgetting
> to do stuff like
No, this was just a hack to get clang to build. I would suggest
changing the constructor just like your patch does, but keeping the
existing ones just forwarding to the new ones.
> NewGV->setThreadLocalMode(OldGV->getThreadLocalMode()) when new
> variables are created based on old ones.
The rest of the review:

+   separated copy of the variable). Optionally, a suggested TLS model may be

Not sure I would call it "suggested". What it is is a promise by the
FE/user that some restrictions apply to how the variable is used:

* localdynamic: Only used from this DSO.
* initialexec: Will not be loaded dynamically.
* localexec: Will be in the executable and is only used from it.

The restrictions should be documented too.

+ bool isThreadLocal() const { return threadLocalMode; }

Add a != NotThreadLocal to make it explicit.

+    default: // Map unknown non-zero value to default.

Why?

+/// Get the IR-specified TLS model for GV, or GeneralDynamic if no model
+/// was selected.

This comment is out of date, no?

+  return TLSModel::GeneralDynamic;

And this return is dead, you can use llvm_unreachable.

Thanks for the tests. Can you please add

* One running llvm-dis
* One targeting Darwin? Just extending X86/tls-models.ll would be fine.
> Thanks,
> Hans
Thanks,
Rafael