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:
- Records has the linkage as extern RecordKeeper Records in Record.h
- Is instantiated as a global in TableGen
- All llvm uses of getAllDerivedDefinitions SEEM to be manifested as a
message to this global RecordKeeper
In short getAllDerivedDefinitions(…) sort of (non-static) treats RecordKeeperas 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
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
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:
- make
- make unittests
- make // with clang
and
- 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
Why are you avoiding it? That's where most of the TableGen tests are (in tests/TableGen). If you only want to run the TableGen tests, you can do:
make TESTSUITE=TableGen check
Ok thanks I'll try that. I was trying to avoid re-building llvm-gcc.
Garrison
Ok thanks I'll try that. I was trying to avoid re-building llvm-gcc.
You don't need llvm-gcc to run most of the "make check" tests. The FrontEnd tests use it, but that's all. If you run configure with "--without-llvmgcc --without-llvmgxx", then it shouldn't use llvm-gcc at all.
Ok, I was wondering why make check-all was using llvm-gcc for clang tests.
Thanks for the pointer, I got to learn about: Release+Asserts/bin/llvm-lit test/TableGen
which is really cool.
Garrison
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
RecordKeeper.patch (557 Bytes)
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
RecordKeeper.3.patch (22.1 KB)
Applied in r121658, thanks Garrison!
-Chris
This looks like great progress to me, applied in r121659.
That said, it is suboptimal for Record to have to have a RecordKeeper& in it. I haven't looked at the code in a long time, is it feasible to detangle that out of record, or is it not worth it?
Another random question: why is createRecord better than using "new Record"? If createRecord is better, it would be good to make the Record ctor private so the code doesn't evolve into sometimes using one and sometimes using the other.
Some minor coding style things:
+ RecordKeeper &getRecords() const {
+ return(TrackedRecords);
+ }
+ Record *createRecord(const std::string &N, SMLoc loc) {
+ return(new Record(N, loc, *this));
+ }
No need for the parens around the return value.
+ RecordKeeper& getRecords() {
+ return(Records);
+ }
Hi Chris,
Thanks for the review!
I believe I caught most of the syntax style issues with the attached patch. It only contains
these style changes.
Hey Chris,
The following patch removes all global references to a RecordKeeper instance for the tblgen
utility.
This looks like great progress to me, applied in r121659.
That said, it is suboptimal for Record to have to have a RecordKeeper& in it. I haven't looked at the code in a long time, is it feasible to detangle that out of record, or is it not worth it?
I'll see if I can come up with another approach. This internal reference was motivated by the
Record::setName(...) and UnOpInit::Fold(...) implementations accessing the previous
global RecordKeeper instance. We could add a RecordKeeper& argument to setName
and OpInit::Fold(...). However as I'm looking at the code, while adding this argument to Fold
for TGParser use is not an issue as it has a RecordKeeper instance, the other Record
classes have a problem because they lack this type of instance. See for example the
implementation of Record.cpp:OpInit::resolveBitReference(...). These methods (such as
resolveBitReference(...) and resolveReferences(...) need a context containing a RecordKeeper
for their evaluation. A cursory glance for the uses of Record::setName(...) seems to imply that adding a
RecordKeeper& argument would not be an issue. I'll keep on looking for other mechanisms.
Another random question: why is createRecord better than using "new Record"? If createRecord is better, it would be good to make the Record ctor private so the code doesn't evolve into sometimes using one and sometimes using the other.
This was another syntactic hack. I personally prefer the factory approach, but I cannot
as it stands get rid of the explicit Record constructor because of MultiClass. It has member
variable of type Record and constructs this member directly using the Record constructor.
It therefore does not need the allocation provided by createRecord. I could push the Record
constructor to protected or private and make MultiClass a friend. This is ugly so I think I should
drop the factory method in favor of new Record. Like I said the factory method is mainly sugar
anyway.
I'm assuming your ok with dropping the RecordKeeper::createRecord(...). I'll send that
patch tomorrow.
Thanks again for taking time to review.
Garrison
RecordKeeper.4.patch (4.56 KB)
Concerning the RecordKeeper reference in Record. Would you prefer to partially go
back to a more limited constrained version of a global. Since we are not threaded anyway,
we could turn the reference into a singleton for the duration of an initial parse and use session.
The concept would be to instead make the reference a static pointer, make RecordKeeper
a friend of Record, and add RecordKeeper::resetSession(void). resetSession would set
the static pointer to itself. Asserts would also have to be added to the methods which needed
a Record to have a non-NULL RecordKeeper pointer. Although we would in effect still be
using a global, we would be allowing it to be controlled while simultaneously removing the
sizeof(RecordKeeper*) number of bytes from Record instances. Offsets into a another say
SmallVector instance could also be attempted, but this seems messy to me.
I am of course assuming that your use of the word suboptimal was concerned with the added
size to Record.
Anyway just a thought
Garrison
The attached patch removes Record::createRecord(...) and its uses from trunk, and includes
previous code style fixes.
Of course it would help if I added the patch file.
RecordKeeper.5.patch (6.26 KB)
I believe I caught most of the syntax style issues with the attached patch. It only contains
these style changes.
Thanks! I applied your followup patch in r121837.
Concerning the RecordKeeper reference in Record. Would you prefer to partially go
back to a more limited constrained version of a global. Since we are not threaded anyway,
we could turn the reference into a singleton for the duration of an initial parse and use session.
The concept would be to instead make the reference a static pointer, make RecordKeeper
a friend of Record, and add RecordKeeper::resetSession(void). resetSession would set
the static pointer to itself. Asserts would also have to be added to the methods which needed
a Record to have a non-NULL RecordKeeper pointer. Although we would in effect still be
using a global, we would be allowing it to be controlled while simultaneously removing the
sizeof(RecordKeeper*) number of bytes from Record instances. Offsets into a another say
SmallVector instance could also be attempted, but this seems messy to me.
I prefer to make RecordKeeper not a global. This leaves us with three options: 1) keep the ivar reference, 2) pass the reference into any method that (transitively) needs the recordkeeper. 3) do #2, but refactor code so that it doesn't have to be passed around as much.
I honestly don't know what is best, if you think the current solution is good enough, lets stay with it. Tblgen isn't exactly a paragon of high performance or low memory use anyway
Thanks Garrison!
-Chris
Thanks Chris,
I can't really comment on the direction because my understanding is still syntax based. I
don't have a comfortable grasp yet of tblgen beyond the llvm doc, how the results are used
in Clang--I'm still very ignorant of llvm backend generation (the real purpose), and the code
calling paths. When I made the mod to Record, adding the RecordKeeper pointer, it did not
even dawn on me that it was a issue. I was under the impression that the number of
records created was ~O(10^2), and that they were not used inline to the rest of the system.
Your comment though made me think that my assumptions were incorrect. So this has to be
your call.
I do have one opinion though. I believe that tblgen doc needs to be completed, or at least
helped along, and thus thought that allowing one to create their own tblgen backend would
go some ways to help with its understanding. A couple of developer's who use it ,indicated
to me that there was there was still an aura of magic (my words), to how certain tblgen backends
interpreted a record's values. Of course this is more of a comment on these backend emitters
than on tblgen's core which is pretty straight forward. Regardless one way to allow developer's
to create their own backends for play, or other purposes would be to create some sort of
plugin architecture. And one way to implement such an architecture is via dlopen or in llvm
portable parlance: llvm::sys::DynamicLibrary::LoadLibraryPermanently(...) (looking a PluginLoader
now), triggered by an option to tblgen. I personally am exploring such a mechanism for
clang AST translation purposes--tblgen generated code probably used in conjunction with
clang::RecursiveASTVisitor<...>.
Is this kind of mod (adding plugins), viable for tblgen?
And tblgen is cool, all we have to do is add an evaluation model, and we have ... Some
of my newly acquainted anti turning complete compatriots will now skewer me.
Anyway thanks again for the time and your tolerance for my verbosity.
Garrison