----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2117/#review4830 -----------------------------------------------------------
TO ANYONE INTERESTED IN USING McPAT IN THE FUTURE: Given that this patch is massive, it may seem daunting to review all of it. To try to address this, I've included a suggested change to the commit message, which articulates the higher-level changes we've introduced. We would greatly appreciate feedback on whether these directions are desired and okay to commit. Suggested additions to the commit message: " These changes include: 1) making XML parsing modular and hierarchical: - shift parsing responsibility into the components - read XML in a (mostly) context-free recursive manner so that McPAT input files can contain arbitrary component hierarchies 2) making power, energy and area calculations a hierarchical and recursive process - components track their subcomponents and recursively call compute functions in stages - make C++ object hierarchy reflect inheritance of classes of components with similar structures - simplify computeArea() and computeEnergy() functions to eliminate successive calls to calculate separate TDP vs. runtime energy - remove Processor component (now unnecessary) and introduce a more abstract System component 3) standardizing McPAT output across all components - use a single, common data structure for storing and printing McPAT output - recursively call print functions through component hierarchy 4) For caches, allow splitting data array and tag array reads and writes for better accuracy 5) Minor: impose more rigorous code style for clarity (more work still to be done) Overall, these changes greatly reduce the amount of replicated code, and they improve McPAT runtime and decrease memory footprint. " I have a few, mostly minor requested changes below: ext/mcpat/basic_components.h <http://reviews.gem5.org/r/2117/#comment4535> Eventually, I think these basic_components.* files should be broken up to better reflect the component modularity/hierarchy by moving the parameter and stats classes into the source files with the components that use them (e.g. CoreParameters should be moved). Please add a comment here to reflect that: "TODO: Since revisions to McPAT aim to make the component hierarchy more modular, many of the parameter and statistics classes/structs included in this file should be moved to the files for their respective components." ext/mcpat/basic_components.h <http://reviews.gem5.org/r/2117/#comment4531> Please add comments for this class: "An object to store the computed data that will be output from McPAT on a per-component-instance basis. Currently, this includes the amount of storage that the component comprises, its chip area, and power and energy calculations." ext/mcpat/basic_components.h <http://reviews.gem5.org/r/2117/#comment4534> Please add comments for this class: "A McPATComponent encompasses all the parts that are common to any component for which McPAT may compute and print power, area, and timing data. It includes a pointer to the XML data from which the component gathers its input parameters, it stores the variables that are commonly used in all components, and it maintains the hierarchical structure to recursively compute and print output. This is a base class from which all components should inherit these functionality (possibly through other descended classes." ext/mcpat/basic_components.h <http://reviews.gem5.org/r/2117/#comment4532> Hmmm... Based on more recent changes to gem5, some of these variables probably shouldn't be static class variables. In particular, the target clock rates and cycle counts will probably need to be modulated on a per-frequency-domain basis. This would be a lot of work immediately, but perhaps we should separate these two variables from this group and add a comment that they should be abstracted eventually. ext/mcpat/basic_components.h <http://reviews.gem5.org/r/2117/#comment4533> I'm not sure this group of variables should be in the MCPATComponent. BITS_PER_BYTE, CAM_ASSOC, and MIN_BUFFER_SIZE are globally used constant variables, so they should probably be defined in the global namespace (maybe as #define macros?). In addition to expanding "OPS" to "OPERANDS", I think NUM_SOURCE_OPS and NUM_INT_INST_SOURCE_OPS should be moved out to core.h. ext/mcpat/cacti/Ucache.h <http://reviews.gem5.org/r/2117/#comment4539> After perusing the mcpat/cacti/* file changes, it appears that most of these changes are code formatting, so I'm not sure that they are necessary. The Cacti code included with McPAT is an already-lightly-modified version of the external tool (v6.5 I believe?), and there are arguments to be made for avoiding farther modification. For example, it may be desirable to incorporate a newer version of Cacti into McPAT at a later time, and more divergence from the Cacti point release will make it harder to identify the functional changes that may need to be resolved for McPAT. *NOTE: It's possible that I've missed necessary functional changes to Cacti in this review, so a more careful review may be necessary. Does anyone else have input on whether to make the Cacti changes? ext/mcpat/cacti/io.cc <http://reviews.gem5.org/r/2117/#comment4540> Just a note: This is an example of Cacti<->McPAT functionality that I think it is fine to clean up per this patch. ext/mcpat/core.h <http://reviews.gem5.org/r/2117/#comment4537> This may be a fair amount of work to change now, but I think it's worth considering for this patch: The prior use of 'ithCore' was specifically for tracking components using an array of structures organization (e.g. indexing into an array of MemManUs). Now that the component hierarchy is defined recursively, these ithCore/ithCore_ variables should no longer be necessary. It is quite confusing to still have them floating around, and in most cases, it looks like it should be simple to just remove them. Can you try eliminating them for this patch? ext/mcpat/logic.h <http://reviews.gem5.org/r/2117/#comment4538> Please add a comment here: "TODO: compute() completes work that should be completed in computeArea() and computeEnergy() recursively. Consider shifting these calculations around to be consistent with rest of hierarchy" - Joel Hestness On Dec. 11, 2013, 10:48 p.m., Yasuko Eckert wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2117/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2013, 10:48 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 9997:8d8c8fd6f702 > --------------------------- > ext: McPAT interface changes and fixes > This patch includes software engineering changes and some generic bug fixes > Joel Hestness and Yasuko Eckert made to McPAT 0.8. There are still known > issues/concernts we did not have a chance to address in this patch. > > > Diffs > ----- > > ext/mcpat/processor.h PRE-CREATION > ext/mcpat/noc.cc PRE-CREATION > ext/mcpat/noc.h PRE-CREATION > ext/mcpat/memoryctrl.cc PRE-CREATION > ext/mcpat/memoryctrl.h PRE-CREATION > ext/mcpat/mcpatXeonCore.mk PRE-CREATION > ext/mcpat/mcpat.mk PRE-CREATION > ext/mcpat/main.cc PRE-CREATION > ext/mcpat/logic.cc PRE-CREATION > ext/mcpat/logic.h PRE-CREATION > ext/mcpat/iocontrollers.cc PRE-CREATION > ext/mcpat/iocontrollers.h PRE-CREATION > ext/mcpat/interconnect.cc PRE-CREATION > ext/mcpat/interconnect.h PRE-CREATION > ext/mcpat/globalvar.h PRE-CREATION > ext/mcpat/core.cc PRE-CREATION > ext/mcpat/common.h PRE-CREATION > ext/mcpat/core.h PRE-CREATION > ext/mcpat/cacti/wire.cc PRE-CREATION > ext/mcpat/cacti/wire.h PRE-CREATION > ext/mcpat/cacti/uca.cc PRE-CREATION > ext/mcpat/cacti/uca.h PRE-CREATION > ext/mcpat/cacti/technology.cc PRE-CREATION > ext/mcpat/cacti/subarray.cc PRE-CREATION > ext/mcpat/cacti/subarray.h PRE-CREATION > ext/mcpat/cacti/router.cc PRE-CREATION > ext/mcpat/cacti/router.h PRE-CREATION > ext/mcpat/cacti/parameter.cc PRE-CREATION > ext/mcpat/cacti/parameter.h PRE-CREATION > ext/mcpat/cacti/nuca.cc PRE-CREATION > ext/mcpat/cacti/nuca.h PRE-CREATION > ext/mcpat/cacti/mat.cc PRE-CREATION > ext/mcpat/cacti/mat.h PRE-CREATION > ext/mcpat/cacti/io.cc PRE-CREATION > ext/mcpat/cacti/htree2.cc PRE-CREATION > ext/mcpat/cacti/htree2.h PRE-CREATION > ext/mcpat/cacti/decoder.cc PRE-CREATION > ext/mcpat/cacti/decoder.h PRE-CREATION > ext/mcpat/cacti/crossbar.cc PRE-CREATION > ext/mcpat/cacti/crossbar.h PRE-CREATION > ext/mcpat/cacti/const.h PRE-CREATION > ext/mcpat/cacti/component.cc PRE-CREATION > ext/mcpat/cacti/component.h PRE-CREATION > ext/mcpat/cacti/cacti_interface.cc PRE-CREATION > ext/mcpat/XML_Parse.h PRE-CREATION > ext/mcpat/XML_Parse.cc PRE-CREATION > ext/mcpat/array.h PRE-CREATION > ext/mcpat/array.cc PRE-CREATION > ext/mcpat/basic_components.h PRE-CREATION > ext/mcpat/basic_components.cc PRE-CREATION > ext/mcpat/bus_interconnect.h PRE-CREATION > ext/mcpat/bus_interconnect.cc PRE-CREATION > ext/mcpat/cachearray.h PRE-CREATION > ext/mcpat/cachearray.cc PRE-CREATION > ext/mcpat/cachecontroller.h PRE-CREATION > ext/mcpat/cachecontroller.cc PRE-CREATION > ext/mcpat/cacheunit.h PRE-CREATION > ext/mcpat/cacheunit.cc PRE-CREATION > ext/mcpat/cacti/Ucache.h PRE-CREATION > ext/mcpat/cacti/Ucache.cc PRE-CREATION > ext/mcpat/cacti/arbiter.cc PRE-CREATION > ext/mcpat/cacti/bank.h PRE-CREATION > ext/mcpat/cacti/bank.cc PRE-CREATION > ext/mcpat/cacti/basic_circuit.h PRE-CREATION > ext/mcpat/cacti/basic_circuit.cc PRE-CREATION > ext/mcpat/cacti/cacti_interface.h PRE-CREATION > ext/mcpat/processor.cc PRE-CREATION > ext/mcpat/sharedcache.h PRE-CREATION > ext/mcpat/sharedcache.cc PRE-CREATION > ext/mcpat/system.h PRE-CREATION > ext/mcpat/system.cc PRE-CREATION > ext/mcpat/technology_xeon_core.cc PRE-CREATION > ext/mcpat/xmlParser.h PRE-CREATION > ext/mcpat/xmlParser.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/2117/diff/ > > > Testing > ------- > > > Thanks, > > Yasuko Eckert > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
