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

[Hans Wennborg]

Thanks for the review! Comments inline; new patch attached.

On Thu, Jun 21, 2012 at 8:40 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> 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.

Right. Doing that.

>>> *) 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' :-)

OK, let's go with GeneralDynamicTLSModel then.

> +   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.

I'm not sure how much detail we should go into here, because the
restrictions might vary depending on the environment. For example,
with glibc, it will be possible to use initial-exec in a .so that will
be loaded dynamically, given that the amount of tls memory is small
enough, even though this doesn't work in general.

I think it's better if we can have the docs say that these correspond
to the TLS models as described in tls.pdf. I've tried to update the
text to do that.

> + bool isThreadLocal() const { return threadLocalMode; }
>
> Add a != NotThreadLocal to make it explicit.

Done.

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

Lots of other functions in the file do this, for example
GetDecodedLinkage and GetDecodedVisibility.

> +/// Get the IR-specified TLS model for GV, or GeneralDynamic if no model
> +/// was selected.
>
> This comment is out of date, no?

Updated.

> +  return TLSModel::GeneralDynamic;
>
> And this return is dead, you can use llvm_unreachable.

It's not dead when GV isn't a GlobalVariable. (I'm not sure that can
happen, though?)

> Thanks for the tests. Can you please add
>
> * One running llvm-dis

Done.

> * One targeting Darwin? Just extending X86/tls-models.ll would be fine.

Done.


Thanks,
Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls_models3.diff
Type: application/octet-stream
Size: 44613 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120622/f841aa8c/attachment.obj>