On Thu, Jun 7, 2012 at 10:02 PM, Chandler Carruth <[email protected]> wrote: > On Wed, Jun 6, 2012 at 9:01 AM, Hans Wennborg <[email protected]> wrote: >> >> On Wed, Jun 6, 2012 at 10:57 AM, Chandler Carruth <[email protected]> >> wrote: >> > On Wed, Jun 6, 2012 at 2:17 AM, Hans Wennborg <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
