> 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.
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. - 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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
