Hi Joel, The patch adds convenience as you point out. Please let us add it.
The patch that removed the GenericMachineType was checked in without discussion or receiving any ship its (see: http://reviews.gem5.org/r/1899/). That is not the community setting a precedent. Sure we all should have done better staying on top of those reviews back in 2013. However, that doesn't mean we should move forward with restricting developers on what information can be passed between ruby and the .sm files. This is simulator. Let's make it as easy to use as possible. More recently, there was an exchange of emails between various gem5 developers prior to this patch (see attachment). There was some consensus that led to this patch, however I realize that you did not participate in that conversation. Thanks, Brad -----Original Message----- From: gem5-dev [mailto:[email protected]] On Behalf Of Joel Hestness Sent: Friday, May 29, 2015 9:08 AM To: gem5 Developer List Subject: Re: [gem5-dev] Review Request 2551: ruby: slicc: have a static MachineType Hey Brad, The definition of MachineType is not only needed for protocol specific > analysis within Ruby, but it needed for protocol specific mapping functions. > > I would like to point out that today, the function > map_Address_to_Directory in RubySlicc_ComponentMapping.hh requires > that all protocols define a Directory machine. By adding this patch, > that requirement will no longer be necessary. It will also allow > folks to add different protocol specific mapping functions without > breaking all other protocols. > You are suggesting that one instance of this should merit changing the whole codebase - I disagree. You should note that the map_Address_to_Directory function is a convenience so that .sm files need not create the MachineID to send to a directory. It would be very straightforward to remove the reference to MachineType_Directory in RubySlicc_ComponentMapping.hh and make .sm files generate the MachineID; map_Address_to_Directory would just return the node ID. If someone wanted to create a snooping Ruby protocol, this would be a good first step. Further, there is significant precedent that gem5 has been moving toward MachineType independence in protocol-independent code. We removed the GenericMachineType, dynamically generate the MachineTypes enum, and removed references to protocol-specific MachineTypes in the sequencer. Let's be forward-thinking about the codebase rather than revisionist. Let's not insist that every feature added to Ruby must have a mainline > use. The public tree should prioritize being a useful baseline that > others can build on top of rather than trying to achieve "perfection". > I agree with you that the public tree should be easily extensible, but it should also prioritize guiding users toward appropriate use of the existing infrastructure. In this case, there are no necessary mainline uses and poorly understood non-mainline uses for protocol-specific MachineType handling in protocol-independent code. We have also discussed that protocol-specific profiling can be supported by adding stats in SLICC. Stats would be a far more extensible solution than allowing protocol-specific MachineTypes to permeate the codebase. It is in the community's interest to add stats to SLICC. There needs to be forcing functions to make these things happen somewhere. Joel -----Original Message----- > From: gem5-dev [mailto:[email protected]] On Behalf Of Joel > Hestness > Sent: Monday, May 11, 2015 11:15 AM > To: gem5 Developer List > Subject: Re: [gem5-dev] Review Request 2551: ruby: slicc: have a > static MachineType > > Hi guys, > More thoughts are inlined below: > > > Hi guys, > >> > >> @Nilay: Thanks for clarifying the desired MachineType usage. > >> > >> I understand the aims now, and I still disagree with statically > >> defined MachineTypes. SLICC is already very powerful, and it only > >> makes sense to add features, not contract them. In this case, scons > >> and SLICC already do the following for us: (1) they allow > >> dynamically extending the list of protocols in any SConscript > >> ('all_protocols' > >> variable), (2) they traverse all directories to find all > >> protocol-related files, and (3) they generate all the MachineType code. > This is nearly everything we need! > >> > >> So, it seems that we're divided on whether we should "just make > >> MachineTypes work" or "do this the right way". I spent a little > >> time today to assemble a patch that dynamically generates the > >> MachineType enum from the superset of all protocol controller > >> definitions. Here it > is: > >> http://reviews.gem5.org/r/2773/ > >> > >> Overall, I feel that the vision should be that SLICC be able to > >> generate and build any/all protocols in a single build operation > >> (like gem5's SE+FS mode merge). I suspect that direction will > >> involve eliminating statically defined types, since they often need > >> to be updated per-protocol, which tends to cause merge headaches. > >> > >> Joel > >> > >> > >> > > > > Joel, AMD might have used machine types for which no definition > > exists in any of the protocols. This used to happen in erstwhile > GenericMachineType. > > > > I'm not necessarily advocating for my patch, but we shouldn't allow > code that functions on non-existent types. Such would be dead code and > doesn't have a place in the mainline repo. Some GenericMachineTypes > may have gone unused, but only because they were statically defined. > Dynamically defined types would eliminate the existence of dead types. > > > I am not in favor of committing Joel's patch or the one I wrote (which > is > > why I had taken it off the reviewboard). We should not add protocol > > specific code to files that are not protocol-specific. Such code > > should reside in protocol specific .sm files. If required, SLICC > > should be modified. I am unable to think of any technical > > difficulty that prevents us from adding statistical variables to .sm > > files. We already have cache hits and misses being collected in .sm files. > > > > I am not in favor of the argument that we should stick to same way > > as things were done in past. If in past something was done > > incorrectly / inefficiently / not-correct-completely, we should be > > willing to change > it. > > In the current context, protocol-specific code in > > protocol-independent files was not the right thing to do. > > Therefore, we should be unwilling to allow such code. > > > After reviewing this thread and the prior thread on protocol-specific > profiling > (https://www.mail-archive.com/[email protected]/msg13084.html), > I agree with Nilay that we should disallow protocol-specific activity > in protocol-independent code. Currently we don't have any such > activity in mainline gem5, and we don't have a well-defined use case > for it that we can review. If we think there is a real need for it, > I'd request that the code be posted for us to review. Having specific > use cases would be helpful in deciding whether/how to manage it. > > To riff on Nilay's suggestions for modifying SLICC: It appears it > would be straightforward to add protocol-specific profiling within > protocol files by extending SLICC to add gem5 stats. While it would be > more complex, it even looks feasible to add enough machine inheritance > that different protocols could share common profiling functionality > (we could also share other things if we were so inclined). > > Joel > > > -- > 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 > _______________________________________________ > 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 _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
