Is there a reason that RecordKeeper:: getAllDerivedDefinitions(...) implementation accesses the global Records instance instead of just referencing itself? As far as I can tell from the usage: 1) Records has the linkage as extern RecordKeeper Records in Record.h 2) Is instantiated as a global in TableGen 3) All llvm uses of getAllDerivedDefinitions SEEM to be manifested as a message to this global RecordKeeper In short getAllDerivedDefinitions(...) sort of (non-static) treats RecordKeeper as a singleton but it is never accessed this way. It is always accessed via the global: Records. Pointers to what I'm missing would be helpful. Thanks in advance Garrison -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101209/1684a5e4/attachment.html>
On Dec 9, 2010, at 4:50 AM, Garrison Venn wrote:> Is there a reason that RecordKeeper:: getAllDerivedDefinitions(...) implementation > accesses the global Records instance instead of just referencing itself? > > As far as I can tell from the usage: > > 1) Records has the linkage as extern RecordKeeper Records in Record.h > 2) Is instantiated as a global in TableGen > 3) All llvm uses of getAllDerivedDefinitions SEEM to be manifested as a > message to this global RecordKeeper > > In short getAllDerivedDefinitions(...) sort of (non-static) treats RecordKeeper > as a singleton but it is never accessed this way. It is always accessed via > the global: Records.Hi Garrison, There is no good reason. This is one of many instances of tblgen just being poorly designed because noone gives it much love. It would be great if you could send in or commit a patch to tidy this up, thanks! -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101209/a8b86a52/attachment.html>
Hey Chris, Ok, so I'm working on creating this trivial patch, for starters, but I'm trying to identify a controlled unit test for tblgen. So all of the following test tblgen to various extents: 1) make 2) make unittests 3) make // with clang and 4) make check // which I'm avoiding Since I'm just starting to understand tblgen, I'm not comfortable with any of these approaches (although I'm currently using make, with clang, and make unittests). I did not find any td files under unittests, and therefore am of the opinion such a direct unit test does not exist. Is this correct? The end result is that I'll be submitting the patch to the group so someone else can commit it, and I will do this when I find out what is going on with these new clang enum comparison warnings. Sorry for the noise for such an extremely trivial patch, but I'm rusty, and tblgen is new to me. My current short term goal is to understand tblgen, by getting rid of its global dependencies, and subsequently modify it to use a portable dlopen like semantic so that one can plugin tblgen backends. Does this exist already? Is anyone else interested in this? I'm interested in backends that accomplish results similar to those generated by clang's tblgen use. I think the options framework will work with a delayed option instantiation (through dlopen). I'm thinking one command line flag would trigger, the dlopen of the specified library, and subsequent flags would specify the action to take, each library having its own globally defined options. Could be some interference here from command line arguments not understood by the initial option action list, but I'm under the current impression that at least the global holding the option list (RegisteredOptionList) can be modified after the fact. I'll check further when I get there. Now if only work would get out of the way. ;-) Garrison On Dec 9, 2010, at 18:32, Chris Lattner wrote:> > On Dec 9, 2010, at 4:50 AM, Garrison Venn wrote: > >> Is there a reason that RecordKeeper:: getAllDerivedDefinitions(...) implementation >> accesses the global Records instance instead of just referencing itself? >> >> As far as I can tell from the usage: >> >> 1) Records has the linkage as extern RecordKeeper Records in Record.h >> 2) Is instantiated as a global in TableGen >> 3) All llvm uses of getAllDerivedDefinitions SEEM to be manifested as a >> message to this global RecordKeeper >> >> In short getAllDerivedDefinitions(...) sort of (non-static) treats RecordKeeper >> as a singleton but it is never accessed this way. It is always accessed via >> the global: Records. > > Hi Garrison, > > There is no good reason. This is one of many instances of tblgen just being poorly designed because noone gives it much love. It would be great if you could send in or commit a patch to tidy this up, thanks! > > -Chris >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101210/32530be3/attachment.html>
Attached patch for removing for the first trivial RecordKeeper:: getAllDerivedDefinitions(...) dependency on the global RecordKeeper Records instance. The real work will be removing this dependency from the other Record.h classes/structs, and from TGParser. Thinking this would be implemented as some sort of context structure/class holding a RecordKeeper instance passed to TGParser which in turn would set this RecordKeeper instance on the various Record classes/ structures. Not sure about this yet. Free to direct if you wish. :-) I checked this with make (with clang), and Release+Asserts/bin/llvm-lit test/TableGen. Garrison On Dec 9, 2010, at 18:32, Chris Lattner wrote:> > On Dec 9, 2010, at 4:50 AM, Garrison Venn wrote: > >> Is there a reason that RecordKeeper:: getAllDerivedDefinitions(...)implementation >> accesses the global Records instance instead of just referencing itself? >> >> As far as I can tell from the usage: >> >> 1) Records has the linkage as extern RecordKeeper Records in Record.h >> 2) Is instantiated as a global in TableGen >> 3) All llvm uses of getAllDerivedDefinitions SEEM to be manifested as a >> message to this global RecordKeeper >> >> In short getAllDerivedDefinitions(...) sort of (non-static) treats RecordKeeper >> as a singleton but it is never accessed this way. It is always accessed via >> the global: Records. > > Hi Garrison, > > There is no good reason. This is one of many instances of tblgen just being poorly designed because noone gives it much love. It would be great if you could send in or commit a patch to tidy this up, thanks! > > -Chris >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101210/95a98efa/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: RecordKeeper.patch Type: application/octet-stream Size: 557 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101210/95a98efa/attachment.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101210/95a98efa/attachment-0001.html>
Hey Chris, The following patch removes all global references to a RecordKeeper instance for the tblgen utility. This effort was motivated by the FIXME remark, in TableGen.cpp. Although a few files were touched, the main change was to Record.h. The patch takes the simple approach of adding a RecordKeeper reference to TGParser, and any needed emitter helper classes. In addition, since some of the classes, and methods in Record reference the previously global RecordKeeper instance, I added another RecordKeeper reference to Record, along with such a typed argument to Record's constructor, and a corresponding factory method to RecordKeeper. Do you want to proceed with this approach, or are there any tweaks you want made? Garrison PS: This patch is standalone with respect to trunk: 121636 -------------- next part -------------- A non-text attachment was scrubbed... Name: RecordKeeper.3.patch Type: application/octet-stream Size: 22641 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101212/a61b1e79/attachment.obj>