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

[Rafael Espíndola]

> 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