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

> 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

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>

> OK, let's go with GeneralDynamicTLSModel then.
OK
>> 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.
It is probably better to be more strict than any particular
implementation. The description on the IL definition is all the
optimizers have to play with. For example, documenting the possibility
of having a small amount of initial-exec in a dso that is dlopend
would be a bad idea, as it would prevent the compiler from lowering a
variable to initial-exec as that might go over the limit.

Something high level like:
> * localdynamic: Only used from this DSO.
> * initialexec: Will not be loaded dynamically.
> * localexec: Will be in the executable and is only used from it.
is probably OK.
>> +    default: // Map unknown non-zero value to default.
>>
>> Why?
>
> Lots of other functions in the file do this, for example
> GetDecodedLinkage and GetDecodedVisibility.
That is peculiar, but you are right, it is better be consistent.
>
>> +  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?)
>
Good point. Looks like it can also be an alias, but we were handling
that only on X86 :-( I have fixed it.

LGTM with the models documented.
>
> Thanks,
> Hans
Thanks,
Rafael