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

[Hans Wennborg]

On Thu, Jun 7, 2012 at 10:02 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Wed, Jun 6, 2012 at 9:01 AM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> 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.
>
>
> Yea, feel free to defer any/all docs changes until you're "done". =] This
> comment wasn't about the patch specifically. Otherwise, your ideas for docs
> seem fine. =]
>
>>
>>
>> > 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.
>
>
> Cool.
>
>>
>> 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?)
>
>
> Bleh, yea skip this. I think the real solution is to bake support for a set
> of string literal values into the attribute system, and have tablegen
> generate the validation code, diagnostics, and mapping to/from an enum. Then
> codegen can just do enum -> LLVM global variable mapping.
>
> But none of that should block this patch, LGTM, submit whenever the LLVM
> side is good-to-go.

Thanks! Landed r159078.

 - Hans