> 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.
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. - Joel ----------------------------------------------------------- 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
