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 (Diff revision 1)

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

Show all issues

ext/mcpat/basic_components.h (Diff revision 1)

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

Show all issues

ext/mcpat/basic_components.h (Diff revision 1)

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

Show all issues

ext/mcpat/basic_components.h (Diff revision 1)

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.

Show all issues

ext/mcpat/basic_components.h (Diff revision 1)

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.

Show all issues

ext/mcpat/cacti/Ucache.h (Diff revision 1)

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?

Show all issues

1. Brad Beckmann Jan. 21, 2014, 11:30 a.m. (Jan. 21, 2014, 11:30 a.m.)

   You bring up a good point here.  I don't think we need to make changes to this current patch, but going forward, we may not necessarily want to make these sorts of chages *if* we end up integrating a newer version of Cacti.  Instead, I'd like to see us concentrate on making sure the interface between Cacti and McPAT is clean and generic.  In the future, we may want to replace Cacti with other models for certain structures.

ext/mcpat/cacti/io.cc (Diff revision 1)

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 (Diff revision 1)

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?

Show all issues

ext/mcpat/logic.h (Diff revision 1)

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"

Show all issues

This is looking good to me. One minor comment below, but I'm happy with this patch.

Would be nice to have another party's input before committing.

ext/mcpat/core.h (Diff revision 2)

After reviewing the use of ithCore, now, I think there is a better way to handle its use here (and possibly in other components); If a component should have an ID number, it should be included in the input XML and be the responsibility of the input generator to assign unique IDs for components as desired. This would continue the push toward completely context-free component instantiation.

That said, I don't think such a change is necessary for this patch. Can you please add a comment here: "TODO: Migrate component ID handling into the XML data to remove this ithCore variable".

Show all issues

Ship It!