Hi Brad, Thanks for the reply. I agree that MachineType needs to be fixed so it can work better with stat collection. However, I disagree that this patch is a step in the right direction. As you say, with this patch any protocol that wants to add a new machine type will have to modify RubySlicc_Export.sm, and this is exactly what I want to avoid. The current state of Ruby is that I can add new machines without having to change anything in the mainline gem5 repo. I strongly want to keep it this way.
If you have future patches that can restore this feature, and do not require new machines to be declared *a priori* in RubySlicc_Export.sm, that's great. But I would much prefer just jumping to those patches, and not going through this patch as it is. The key feature that I want is to be able to create a new machine for some protocol without having to export its name globally to all protocols. If we go with this patch, then anyone who ever creates a new machine type will have to declare it in RubySlicc_Export.sm. That means either in the gem5 repo we'll have all sorts of machine types that only one (or a few) people are using, or everyone who wants to use a new machine type will have to have yet another patch in their patch queue. However, I will admit this isn't certainly the end of the world if this patch goes in. If you guys really need it, we can just add another patch to our internal patch queue for gem5-gpu. I'm mostly pushing back here because of all the times that I didn't push back on similarly (seemingly) small changes to Ruby ;). Cheers, Jason On Fri, May 8, 2015 at 3:10 PM Beckmann, Brad <[email protected]> wrote: > Oops I should have said the patch introduces the MachineType enum, > rather than removes it. Please disregard the third paragraph in my prior > email. > > > > Yes, this patch helps with collecting stats that are protocol specific. > Eventually we need to move the MachineType underneath the RubySystem. I > think this patch will help in that effort. > > > > Brad > > > > > > > > *From:* Beckmann, Brad > *Sent:* Friday, May 08, 2015 1:04 PM > *To:* 'Jason Power'; gem5 Developer List; Nilay Vaish > *Subject:* RE: [gem5-dev] Review Request 2551: ruby: slicc: have a static > MachineType > > > > First off, I was also seeing the private patch error. I think Nilay > changed the access permissions on that particular patch for a moment. It > should now be accessible again. > > > > Let’s be clear, the MachineType object and its associated functions were > static before this change. That is a big problem that we will need to fix > soon. You’ll see more from Brandon Potter on this soon. > > > > I believe this patch actually is a step in the right direction as far as > removing the static dependencies on MachineType. Before this patch, the > MachineType enum needed to be inclusive of all possible machine types > created by the protocols. Thus the RubySlicc_Export.sm file was a global > file that needed to be modified by every protocol that introduced a new > machine name. That dependency goes away with this change. > > > > Yes, we do have patches in the pipeline that implement non-CPU cache > controllers. They depend on this patch being checked in. > > > > Brad > > > > > > > > > > *From:* Jason Power [mailto:[email protected] <[email protected]>] > > *Sent:* Friday, May 08, 2015 11:57 AM > *To:* gem5 Developer List; Beckmann, Brad; Nilay Vaish > > *Subject:* Re: [gem5-dev] Review Request 2551: ruby: slicc: have a static > MachineType > > > > Hi Brad, > > I personally would not like to see this patch pushed as it is. It breaks > our workflow for gem5-gpu. Also, it seems to me that making the > MachineTypes static is a step backwards. It's already hard with Ruby to > create a protocol that isn't a traditional CPU-oriented protocol, and by > not allowing new machine types (easily) it makes it that much harder. > > > > I have to admit it's been a long time since I looked at this patch, and > unfortunately I can't right now because I'm getting an error with > reviewboard "This review request is private. You must be a requested > reviewer, either directly or on a requested group, and have permission to > access the repository in order to view this review request." Strange. > > > > Brad, do you have any patches in the (very near) pipeline that will allow > for more MachineTypes than just the basic CPU-oriented caches? I suppose I > could be convinced to modify our protocols once, but I really don't want to > have to change them with every gem5 update more than we already do. > > > > Overall, I can't remember what problem this patch originally solves. Was > the problem that the Ruby stats depended on a static number of > (Generic)MachineTypes? If this is the problem this patch solves, wouldn't > it be better to instead make the stats accept dynamic MachineTypes? > > > > If the rest of the community disagrees with me and wants this patch as is, > we can deal here at gem5-gpu. I would just much rather keep the changes we > have to the mainline gem5 minimal as merges are already pretty hellish as > it is. > > > > Thanks, > > Jason > > > > On Fri, May 8, 2015 at 12:07 PM Beckmann, Brad <[email protected]> > wrote: > > Hi Nilay, > > Can you check this patch and the previous patch (review 2550) in the > repo? We will be posting a series of patches next week and they depend on > these two patches. > > Thanks, > > Brad > > > > From: Brad Beckmann [mailto:[email protected]] On Behalf Of > Beckmann, Brad > Sent: Monday, April 27, 2015 3:30 PM > To: Nilay Vaish; Beckmann, Brad; Default > Subject: Re: Review Request 2551: ruby: slicc: have a static MachineType > > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2551/ > > > > Ship it! > > Ship It! > > > - Brad Beckmann > > > On December 4th, 2014, 6:08 p.m. UTC, Nilay Vaish wrote: > Review request for Default. > By Nilay Vaish. > > Updated Dec. 4, 2014, 6:08 p.m. > Repository: gem5 > Description > > Changeset 10594:b25131489b95 > > --------------------------- > > ruby: slicc: have a static MachineType > > This patch moves from a dynamically defined MachineType to a statically > > defined one. The need for this patch was felt since a dynamically defined > > type prevents us from having types for which no machine definition may > > exist. > > > > The following changes have been made: > > i. each machine definition now uses a type from the MachineType enumeration > > instead of any random identifier. This required changing the grammar and > the > > *.sm files. > > > > ii. MachineType enumeration defined statically in RubySlicc_Exports.sm. > > > Diffs > > * src/mem/protocol/MESI_Two_Level-L1cache.sm (fea29fc045ee) > * src/mem/protocol/MESI_Two_Level-L2cache.sm (fea29fc045ee) > * src/mem/protocol/MESI_Two_Level-dir.sm (fea29fc045ee) > * src/mem/protocol/MESI_Two_Level-dma.sm (fea29fc045ee) > * src/mem/protocol/RubySlicc_Exports.sm (fea29fc045ee) > * src/mem/slicc/ast/DeclListAST.py (fea29fc045ee) > * src/mem/slicc/ast/MachineAST.py (fea29fc045ee) > * src/mem/slicc/parser.py (fea29fc045ee) > * src/mem/slicc/symbols/SymbolTable.py (fea29fc045ee) > * src/mem/slicc/symbols/Type.py (fea29fc045ee) > > View Diff<http://reviews.gem5.org/r/2551/diff/> > > > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
