----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2773/#review6497 -----------------------------------------------------------
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. - Sooraj Puthoor 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
