[cfe-commits] [Patch] Support the tls_model attribute (PR9788)

[Hans Wennborg]

On Wed, Jun 6, 2012 at 10:57 AM, Chandler Carruth <chandlerc at google.com> wrote:
> On Wed, Jun 6, 2012 at 2:17 AM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> Hi all,
>>
>> Attached is the Clang-side patch for adding support for explicitly
>> selected TLS models [1]. This adds support for the tls_model attribute
>> [2].
>
>
> Very cool.

Thanks for looking! New patch attached.

> Should support for these be documented somewhere? It should at least go into
> the release notes for 3.2.

I've added it to the ReleaseNotes.html. Is there any other place I
should add it to? Maybe LanguageExtensions.html? I was hoping to add
the "-ftls_model=" command-line flag too (in a later patch), and then
document properly.

> This parsing is duplicated a lot:
>
> +    const TLSModelAttr *Attr = D.getAttr<TLSModelAttr>();
> +    if (Attr->getModel() == "global-dynamic") {
> +       TLM = llvm::GlobalVariable::GlobalDynamicTLSModel;
> +    } else if (Attr->getModel() == "local-dynamic") {
> +       TLM = llvm::GlobalVariable::LocalDynamicTLSModel;
> +    } else if (Attr->getModel() == "initial-exec") {
> +       TLM = llvm::GlobalVariable::InitialExecTLSModel;
> +    } else if (Attr->getModel() == "local-exec") {
> +       TLM = llvm::GlobalVariable::LocalExecTLSModel;
> +    } else {
> +      llvm_unreachable("unknown TLS model attribute");
> +    }
>
> Could you pull this into helper function that returns the
> llvm::GlobalVariable::ThreadLocalMode value? If you make it return
> not-thread-local in the event that nothing parses, you can use it in both
> the attribute checking code to diagnose invalid attributes, and in the rest
> of Clang to interpret them.

I've put a helper function in CodeGenModule for now; that way at least
the codegen functions share the code.

To make the attribute checking codes share this, I'm unsure about what
dependencies I'm allowed to add. If I put the helper in Sema, is Sema
allowed to depend on LLVM's GlobalVariable? And is CodeGen allowed to
depend on Sema? (I'm guessing introducing a dependency from Sema to
CodeGen is right out?)

Anyway, the CodeGen bits are cleaner now, and the Sema bit is really
just a simple check.

> +    S.Diag(Attr.getLoc(), diag::err_attr_tlsmodel_arg)
> +        << "\"global-dynamic\", \"local-dynamic\", "
> +        "\"initial-exec\" or \"local-exec\"";
>
> A big string like this should just go directly into the diagnostic text in
> the '.td' file.

Done.

 - Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls_models_clang2.diff
Type: application/octet-stream
Size: 12861 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/2e4bf6ca/attachment.obj>