<div class="gmail_quote">On Wed, Jun 6, 2012 at 9:01 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Wed, Jun 6, 2012 at 10:57 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> On Wed, Jun 6, 2012 at 2:17 AM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>><br>
>> Hi all,<br>
>><br>
>> Attached is the Clang-side patch for adding support for explicitly<br>
>> selected TLS models [1]. This adds support for the tls_model attribute<br>
>> [2].<br>
><br>
><br>
> Very cool.<br>
<br>
</div>Thanks for looking! New patch attached.<br>
<div class="im"><br>
> Should support for these be documented somewhere? It should at least go into<br>
> the release notes for 3.2.<br>
<br>
</div>I've added it to the ReleaseNotes.html. Is there any other place I<br>
should add it to? Maybe LanguageExtensions.html? I was hoping to add<br>
the "-ftls_model=" command-line flag too (in a later patch), and then<br>
document properly.<br></blockquote><div><br></div><div>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. =]</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> This parsing is duplicated a lot:<br>
><br>
> +    const TLSModelAttr *Attr = D.getAttr<TLSModelAttr>();<br>
> +    if (Attr->getModel() == "global-dynamic") {<br>
> +       TLM = llvm::GlobalVariable::GlobalDynamicTLSModel;<br>
> +    } else if (Attr->getModel() == "local-dynamic") {<br>
> +       TLM = llvm::GlobalVariable::LocalDynamicTLSModel;<br>
> +    } else if (Attr->getModel() == "initial-exec") {<br>
> +       TLM = llvm::GlobalVariable::InitialExecTLSModel;<br>
> +    } else if (Attr->getModel() == "local-exec") {<br>
> +       TLM = llvm::GlobalVariable::LocalExecTLSModel;<br>
> +    } else {<br>
> +      llvm_unreachable("unknown TLS model attribute");<br>
> +    }<br>
><br>
> Could you pull this into helper function that returns the<br>
> llvm::GlobalVariable::ThreadLocalMode value? If you make it return<br>
> not-thread-local in the event that nothing parses, you can use it in both<br>
> the attribute checking code to diagnose invalid attributes, and in the rest<br>
> of Clang to interpret them.<br>
<br>
</div>I've put a helper function in CodeGenModule for now; that way at least<br>
the codegen functions share the code.<br></blockquote><div><br></div><div>Cool.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">To make the attribute checking codes share this, I'm unsure about what<br>

dependencies I'm allowed to add. If I put the helper in Sema, is Sema<br>
allowed to depend on LLVM's GlobalVariable? And is CodeGen allowed to<br>
depend on Sema? (I'm guessing introducing a dependency from Sema to<br>
CodeGen is right out?)<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>But none of that should block this patch, LGTM, submit whenever the LLVM side is good-to-go.</div></div>