Hi Brad, I'm willing to discuss this further, though at this point, I am still opposed. I don't really have more to add without more to consider (your attachment didn't come through - please resend).
Also... did you look at my proposed solution ( http://reviews.gem5.org/r/2773/)? It's better than this patch, because it dynamically generates the MachineTypes list from the superset of protocol controllers. This avoids the issue of needing to change a static MachineType enumeration when adding new controllers. Joel On Tue, Jun 2, 2015 at 8:05 PM, Beckmann, Brad <[email protected]> wrote: > 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 > -- 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
