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. On Dec 12, 2010, at 19:30, Chris Lattner wrote:> > On Dec 12, 2010, at 10:54 AM, Garrison Venn wrote: > >> >> 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>> Some minor coding style things: > > ...> -Chris >-------------- next part -------------- A non-text attachment was scrubbed... Name: RecordKeeper.4.patch Type: application/octet-stream Size: 4666 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101212/da76c8f5/attachment.obj>
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 On Dec 12, 2010, at 21:52, Garrison Venn wrote:> 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. > > On Dec 12, 2010, at 19:30, Chris Lattner wrote: > >> >> On Dec 12, 2010, at 10:54 AM, Garrison Venn wrote: >> >>> >>> 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 > >> > >> Some minor coding style things: >> >> ... > >> -Chris >> > > <RecordKeeper.4.patch>
The attached patch removes Record::createRecord(...) and its uses from trunk, and includes previous code style fixes. On Dec 12, 2010, at 21:52, Garrison Venn wrote:> ... > >> >> 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. > > ... >> Some minor coding style things: >> >> ... > >> -Chris >> > > <RecordKeeper.4.patch>I think you will be ok with this. Garrison PS: Standalone patch with respect to trunk 121697
Of course it would help if I added the patch file. :-) On Dec 13, 2010, at 8:49, Garrison Venn wrote:> The attached patch removes Record::createRecord(...) and its uses from trunk, and includes > previous code style fixes. > > On Dec 12, 2010, at 21:52, Garrison Venn wrote: > >> ... >> >>> >>> 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. >> >> ... >>> Some minor coding style things: >>> >>> ... >> >>> -Chris >>> >> >> <RecordKeeper.4.patch> > > I think you will be ok with this. > > Garrison > > PS: Standalone patch with respect to trunk 121697 >-------------- next part -------------- A non-text attachment was scrubbed... Name: RecordKeeper.5.patch Type: application/octet-stream Size: 6406 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101213/c5530b10/attachment.obj>
On Dec 12, 2010, at 8:12 PM, Garrison Venn wrote:>> 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