> On June 18, 2015, 9:38 p.m., Sooraj Puthoor wrote: > > Hi Joel, > > I have some minor concerns and some major concerns about this patch..:). > > Let me start with the minor concerns first: > > > > 1) If this patch is applied on top of Nilay's > > http://reviews.gem5.org/r/2550/ patch, gem5 will not compile with this > > patch. Nilay's patch removes the line "self.machine_types = idents" from > > MachineAST.py; so we will get "AttributeError: 'MachineAST' object has no > > attribute 'machine_types':" error. The fix is to add machine_types back > > into MachineAST. > > 2) Nilay's http://reviews.gem5.org/r/2551/ patch seems to do something > > which is kind of opposite to what this patch is doing. So, we will have to > > discard either this patch or Nilay's patch. > > 3) What happens if the machine is declared like "machine({L1, L2},".. I > > could be wrong but I think this patch cannot take care of this situation. > > > > Now, coming back to major concerns (in increasing order of importance): > > 1) Probably, going through all .sm files in the protocol_source path is not > > an optimal solution. The optimal solution may be going through the .sm > > files defined in protocol.slicc. Now, if I am working on a new protocol and > > has multiple incomplete .sm files under development, this patch goes > > through those incomplete files and may create difficulties for the > > developer. I think the user will have to move those files out of > > protocol_source path or will have to change the extension (isnt it so?) to > > satisfy this patch. > > 2) Most importantly, the developer completely loses the flexibility to > > define custom machine types with this patch. In multiple occasions, the > > same machine is instantiated multiple times in a ruby system and sometimes > > they are differentiated (say for profiling purposes) by writing custom > > functions in .sm files which returns different machine types for different > > instantiations. To elaborate a bit more, assume a situation in which I can > > use the same cache.sm file for both L1 and L2 cache. Also assume that the > > declaration in cache.sm is "machine(Cache,". Now, if I have to distinguish > > these controllers, I can have a function inside the .sm file which returns > > "MachineType:L2" or "MachineType:L1" depending on some variable or event or > > message. Such a scheme will not work with this patch because the > > MachineType enum will only have "Cache" and not "L1" or "L2". With Nilay's > > http://reviews.gem5.org/r/2551/ patch the developer can actually > > instantiate any number of pseudo-machine types. > > > > Having said all this, I understand the utility of this patch but given the > > trade-offs, I think it is better to check in Nilay's patch and discard this > > patch. Please let me know your thoughts. > > Joel Hestness wrote: > None of these are actual problems. > > Your minor "concerns": > 1) Merging will be necessary with other patches, sure. However, > http://reviews.gem5.org/r/2550/ in particular has been discarded, so this is > a non-issue. > 2) This patch was submitted in lieu of http://reviews.gem5.org/r/2551/, > and this is clear from the email trail (here: > http://permalink.gmane.org/gmane.comp.emulators.m5.devel/26215) > 3) This patch works. I've tested it with all protocols in mainline > (including MOESI_hammer). > > Your major "concerns": > 1) This patch only parses the AST of .sm files in the path and then > extracts machine names. If you have a partially complete .sm file, it only > needs to be parseable. Why would someone keep uncompilable .sm files in > protocol directories? > 2) This situation doesn't exist in the current codebase and wasn't > presented as a use case. Further, doing what you describe would likely result > in buggy behavior in the .sm files, because MachineType counts could not > reflect the number of L1 and L2 types, but only the number of Cache types. It > seems like you could only use the L1 and L2 types outside of protocol files > (though MachineTypes are currently defined to be protocol-specific). What is > the purpose of the L1 and L2 types? > > I understand that you're trying to get AMD internal code working with the > changes available in the public repos. However, please let's be more concrete > than just "concerns", because they're either not problems or if they are, > your description is too vague to be addressed. There may be ways for AMD to > address problems internally or ways for this patch to fix them, but we need > to know specifically what they are. > > Sooraj Puthoor wrote: > Thanks for the link to the email. I was not aware of that email, so I did > not know this patch was created to replace Nilay's patch (r/2551). Regarding > my comment on r/2550, I was just highlighting the possibility of potential > merge conflict. > > Coming back to major concerns, probably I did not do a good job in giving > you some concrete example. Let me try this again..:). > 1) As I explicitly mentioned in the original comment, I was advocating > collecting machinetypes only from the specified protocol and not collecting > all available machine types going through all protocol files. The example I > mentioned there was just one scenario which I hoped will help justify my > comment. I think keeping uncompilable .sm files in the protocol path while > new protocol is under development (or even some uncompilable temporarily > deprecated protocol files in the same directory) is a possible scenario. No > better example come to my mind right now. Fundamentally, what I want to point > out here is the compilation should not be affected by issues in the protocol > files which the user is not concerned at all. The compilation should only be > affected by the protocol files of the specific protocol being used. > 2)Probably, I was vague in the original comment. When we combine multiple > cache controllers to a single .sm file (for example, L1 and L2 controllers > combined to one .sm file called machine(Cache:"")); although we have only one > .sm file, we actually have more than one Machine in the system and each > machine should have an enum declaration in the MachineType:Enum list. Since > this patch associates number of machines and MachineType with the machine > declaraitons in .sm file, the developer cannot model combined controllers > with this patch. BTW, I am not talking about MOESI_Hammer protocol kind of > situation which actually has two separate controllers although both of them > are defined in one .sm file. I am talking about integrated controller which > is actually the state machine of two (or more) machines integrated into one > .sm file. With Nilay's patch (r/2551), the developer can add any number of > machine types to the enum list and this enables the developer to have > integrated controllers. The underlying differnce is Nilay's patch doesn't > restrict MachineTypes to the declarations in the .sm files. > > We rely on these features for our internal protocols although this is not > a part of current codebase. So, if you could modify this patch to collect > machinetypes from only the specified protocol (my first point) and if you > could give the flexibility to user to add MachineTypes that are not declared > as machines in the .sm files (my second point), that will add more utility to > this patch and make this patch a true replacement of Nilay's r/2551 patch. > > Joel Hestness wrote: > 1) I understand your concern about uncompilable code, but it seems pretty > exaggerated. In order for a controller file to cause a compilation error with > this patch, it needs (1) the PROTOCOL_NAME to be declared in the scons > all_protocols array, (2) to have a PROTOCOL_NAME.slicc file that points to an > uncompilable controller file, and (3) the controller file is not even > parseable. Your deprecated protocol file example suggests that a user not > only has an unparseable file, but everything else is set up to allow you to > try to compile the protocol. Given that it is very simple to either remove > the PROTOCOL_NAME from all_protocols or remove a patch containing the > protocol altogether, it seems like this is a very minor issue. > > 2) To me, the behavior you're describing indicates that the machine > should be declared with the following header: machine({L1, L2}, ...), because > you are communicating outside of those controllers using those types. With > the current state of SLICC and the scenario you describe, the L1 and L2 > machine types cannot be used by any of the SLICC controller context-sensitive > features like machineCount() or machineIDToMachineType(), because there would > be no machines declared with those types. Thus, you must ONLY be using the L1 > and L2 types to communicate outside of Ruby (e.g. for stats > collection/profiling based on machine ID). By declaring the machine as and > L1/L2 controller, this patch will work just fine with your protocol. You > should probably be declaring the machine as an L1/L2 controller anyway, since > otherwise, you're not actually using them as MachineTypes. > > To be clear, I'm not strongly advocating that we use my proposed patch > here. This was posted as a smarter way to resolve the MachineTypes headaches > of other patches, while allowing AMD to use MachineTypes outside of SLICC > controllers (e.g. for stats). My opinion is that gem5 should not allow > MachineTypes to be exposed outside of SLICC controllers, because there are > currently no known situations where it is absolutely required. For stats > purposes, SLICC should be augmented to allow stats annotations within SLICC > controllers, which should remove the need to communicate MachineTypes outside > of controllers. > > If AMD wants to continue using MachineTypes outside of controllers (a > feature which is basically deprecated) AND gem5 mainline should allow that > feature (questionable), I think the solution should be to reintroduce the > GenericMachineType as an interface between SLICC and non-SLICC code, rather > than stretching the function of MachineType to fit that bill. (I also don't > understand why it's burdensome to just maintain one's own internal patch that > adds the GenericMachineType back).
Hi Joel, Thanks for the suggestions. I think we can remove the deprecated files from the scons PROTOCOL_NAME list and avoid the prtoblem of deprecated files. Regarding the otehr issue of adding machine types to the enum list by the developer, we need that feature for many of our protocols. Would it be possibble to add a "supplementary enum decalaration list" to which the developer can add machinetypes whose definitions may not be there in any .sm files? I think having such a feature will solve the issue which we are having with a strictly "Dynamically find+declare all MachineTypes" approach of this patch. - Sooraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2773/#review6497 ----------------------------------------------------------- On May 9, 2015, 7:29 p.m., Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2773/ > ----------------------------------------------------------- > > (Updated May 9, 2015, 7:29 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10832:ba670c48f3c8 > --------------------------- > ruby: slicc: Dynamically find+declare all MachineTypes > > To avoid statically declaring MachineTypes that can be used by Ruby protocols, > modify the SLICC parser to dynamically collect the different machine types > from all the available protocols to declare them in the MachineTypes generated > enum. This change declares the full set of types, but only generates the > controller files for the specified protocol. > > > Diffs > ----- > > src/mem/protocol/SConscript fbdaa08aaa42 > src/mem/slicc/ast/DeclAST.py fbdaa08aaa42 > src/mem/slicc/ast/DeclListAST.py fbdaa08aaa42 > src/mem/slicc/ast/MachineAST.py fbdaa08aaa42 > src/mem/slicc/parser.py fbdaa08aaa42 > > Diff: http://reviews.gem5.org/r/2773/diff/ > > > Testing > ------- > > Built MI_example and MOESI_hammer in gem5. Also applied a personal patch > that adds MachineType-dependent code to RubyMemoryController, and > successfully built a protocol that does not define machines required by > that that code. Benchmarks run in each build, and MachineType-specific > functionality appears correct. > > > Thanks, > > Joel Hestness > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
