Hi guys, I have some input here: 1) First and foremost: If we decide the direction of this patch is alright, it should *NOT* ship as is, because it only fixes the machine type for the MESI_Two_Level protocol. The rest of the protocol files need to be updated (unless I'm overlooking something).
2) I agree with Jason that we shouldn't have a statically defined set of machine types. The way that Ruby generates controllers is very flexible so that adding or swapping out controllers is simple. You would no longer be able to do this without the MachineType enumeration containing a superset of all the controllers that various protocols include. 3) I've read this request each time discussion has come up, and I still don't understand the need for it. As is, the patch's aim and the protocol-specific stats problem are not adequately described for this patch to make sense. I'd strongly prefer that the patch description be rewritten so that it is much clearer what is meant by "prevents us from having types for which no machine definition may exist". I'd also strongly prefer a more detailed description of the problem with protocol-specific stats. These are a must to offer an informed patch review. Joel On Fri, May 8, 2015 at 3:19 PM, Jason Power <[email protected]> wrote: > 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 > -- Joel Hestness PhD Candidate, Computer Architecture Dept. of Computer Science, University of Wisconsin - Madison http://pages.cs.wisc.edu/~hestness/ _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
