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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
