> On Sept. 20, 2013, 7:46 a.m., Andreas Hansson wrote: > > I do not really see the point here. Could you be more clear around what > > this "integration" would involve? In any case, I would vote not to include > > the McPat source in gem5, and if it really needs to live in the source > > tree, get the users that want to have it to clone/checkout in ext/mcpat. > > Nathan Binkert wrote: > I agree. Shouldn't McPAT be maintained in its own repository? Shouldn't > it just use EXTRAS? > > Brad Beckmann wrote: > Thanks Tony for posting this initial patch. I know it has been a few > weeks, but want to restart this discussion. We would like to include this > version of McPAT directly into gem5 so that we can keep it "in sync" with the > gem5 output. We fear that if we move it to a separate source tree, it will > become stale with the constantly evolving gem5 statistics and configurations. > There are also secondary benefits from AMD's perspective in keeping it the > same respository that I'd rather not get into. > > So what is the benefit of having it in a separate repository? It isn't > that much code and there is already many other external tools/scripts in the > gem5 ext directory. > > Yasuko Eckert wrote: > I wanted to restart this discussion. I know there are many demands from > different people to add McPAT to gem5, so it would be nice to check in this > original McPAT patch as well as three improvement patches from AMD (#2151, > 2117, and 2118) very soon. Could we shoot for the end of the week? > > Andreas Hansson wrote: > Hi Yasuko, > > Thanks again for all the effort. > > I would still like to see if it is really necessary to add all the > sources to the gem5 tree, as opposed to simply cloning/downloading mcpat in a > ext/ folder like we do for e.g. DRAMSim2. I'd imagine #2151 and #2118 could > easily be added next to a stock mcpat. The big question is #2117. As far as I > understand, the patch mainly improves the run-time and memory requirements of > mcpat. How does that compare to the stock mcpat? Also, how does it compare to > mcpat 1.0? I'm personally not familiar with the improvements/differences > between the two versions. > > If #2117 is really critical, perhaps we could consider having the gem5 > build process apply it to the downloaded mcpat (I have proposed this in the > past). > > Brad Beckmann wrote: > There is a strong desire to directly check in a version of McPAT into our > gem5 tree so that the community can make sure that it is maintained. For > instance, it is much easier to use the regression tests if only one > repository is involved. Having a build process that downloads mcpat seems > far too complicated and prone to error. > > So how do we move forward on this? Is there a process we can use to > drive to a consensus? > > Andreas Hansson wrote: > Hi Brad, > > I understand your position, and if I am the only person that does not get > a warm fuzzy feeling then by all means go ahead. > > That said, I still believe that there are reasons to proceed with caution: > > 1) There is a danger in having different mcpat versions in different > places. At least at the moment, everyone is comparing the same (uncorrelated) > numbers. > > 2) If the gem5 branch of mcpat would become the de-facto in the computer > architecture community, then I would at least like to make sure we are not > dropping important bits by going for mcpat 0.8 rather than 1.0 as a starting > point. Can someone enumerate the differences/changes? > > 3) Who is going to maintain the mcpat branch, and who is going to > review/approve any changes? Coming back to (1), if someone wants to mend a > power model, what kind of criteria should be used to accept the changes? Is > one model going to fit everyone? > > 4) With the added DVFS support in gem5, will the "integration" with mcpat > break? I merely want to make sure we're not making life more difficult by > adding too stringent assumptions at this point. > > If you guys step up to own it then (1), (2) and (3) is resolved as far as > I'm concerned. > > As a minor side note, when it comes to the download process, I believe it > was roughly 4 years between the two current versions in circulation. Is that > really such a big problem to maintain? > > Anthony Gutierrez wrote: > I think the main problem is that people are not comparing the same > numbers. There is a large community who use it and make their own > modifications, and because there is no easy way to share the changes, or any > standards, everyone is essentially using their own hacked-up version. While > your concerns about who will maintain it are valid, I think the more > important issue is to have a single version where those who actively use it > can submit changes, and have discussions about what should or should not be > added. > > Joel Hestness wrote: > I am a strong proponent for having a McPAT repo for the reasons Brad and > Anthony have articulated. There are certainly too many versions of McPAT with > various different models running around. Further, since I modified McPAT at > AMD, I have receive innumerable requests to share the code, so I know that > there is significant demand to have it in a repo (I'd estimate at least 5x > the demand that you're hearing in this review process). > > That said, I understand Andreas' hesitation to including McPAT within > gem5; We already see, on average, about one review request/update per day for > gem5, and with all the changes that people may want to make to McPAT, there > is potential to have a significantly more review traffic to weed through > unless handled appropriately (e.g. perhaps we'd need a gem5+McPAT Reviewboard > group?). > > To address Andreas' prompts: > 1) People want an agreed-upon version, and a repo would significantly > improve the current status quo. We need a repo - it's just not clear whether > it should be inside or outside gem5 > > 2) The primary mods from McPAT 0.8 to 1.0 are: > - Exposing Vdd as a component parameter in cases where it may be > adjustable (see bullet 4) > - Updated Cacti interface and components to support preliminary > power-gating functionality (see bullet 4) > - Additional transistor feature sizes > > 3) The challenges you list would need to be worked out, but without a > repo, we can only speculate on how we'd address the hard ones. Here are a few > need-to-have criteria: > - Changes that improve code style without changing output values > - Bug fixes that do not change output values > - Changes that improve software architecture (extensibility, > abstraction, etc.) without changing output values > - New functionality that has been validated or based on other public > releases (e.g. merging McPAT 1.0 changes) > - The most tricky one will be modifying power models, and this will > require crossing a couple bridges before we really understand how this should > be done > > 4) Just as gem5 needed mods for DVFS, McPAT will also need mods. These > mods will probably need to merge some of the changes from McPAT 1.0. This > shouldn't block us from creating a repo now. > > > Overall, despite identifying with many of Andreas' concerns, my opinion > is that we need to include McPAT in gem5/ext/ in order to encourage gem5 > developers to be aware of the available power models and to promote the > co-development of McPAT with gem5.
Thanks a lot for the elaborate response Joel. As I said before, don't let me stop you. When it comes to the review load, I think it's wishful thinking, but let's cross our fingers people actually want to contribute their modifications :-). Any changes should indeed be binned into the five categories you identify, and we probably need to invent some new keywords and identify some gate-keepers. Concerning the DVFS support, does it mean that a mcpat added today (as of the patch in question) is incomplete in the sense that it won't work with the current gem5 (that already supports DVFS, hierarchical clock and power domains etc.)? - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2021/#review4721 ----------------------------------------------------------- On Dec. 9, 2013, 10:49 p.m., Anthony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2021/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2013, 10:49 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 9994:23fa89b810a1 > --------------------------- > ext: add McPAT source > > this adds the source for mcpat, a power, area, and timing modeling framework. > this will allow for the future integration of mcpat into gem5. > > > Diffs > ----- > > ext/mcpat/ARM_A9.xml PRE-CREATION > ext/mcpat/ARM_A9_2000.xml PRE-CREATION > ext/mcpat/ARM_A9_800.xml PRE-CREATION > ext/mcpat/Alpha21364.xml PRE-CREATION > ext/mcpat/Niagara1.xml PRE-CREATION > ext/mcpat/Niagara1_sharing.xml PRE-CREATION > ext/mcpat/Niagara1_sharing_DC.xml PRE-CREATION > ext/mcpat/Niagara1_sharing_SBT.xml PRE-CREATION > ext/mcpat/Niagara1_sharing_ST.xml PRE-CREATION > ext/mcpat/Niagara2.xml PRE-CREATION > ext/mcpat/Penryn.xml PRE-CREATION > ext/mcpat/README PRE-CREATION > ext/mcpat/XML_Parse.h PRE-CREATION > ext/mcpat/XML_Parse.cc PRE-CREATION > ext/mcpat/Xeon.xml PRE-CREATION > ext/mcpat/arch_const.h 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/cacti/README PRE-CREATION > ext/mcpat/cacti/Ucache.h PRE-CREATION > ext/mcpat/cacti/Ucache.cc PRE-CREATION > ext/mcpat/cacti/arbiter.h PRE-CREATION > ext/mcpat/cacti/arbiter.cc PRE-CREATION > ext/mcpat/cacti/area.h PRE-CREATION > ext/mcpat/cacti/area.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/batch_tests PRE-CREATION > ext/mcpat/cacti/cache.cfg PRE-CREATION > ext/mcpat/cacti/cacti.i PRE-CREATION > ext/mcpat/cacti/cacti.mk PRE-CREATION > ext/mcpat/cacti/cacti_interface.h PRE-CREATION > ext/mcpat/cacti/cacti_interface.cc PRE-CREATION > ext/mcpat/cacti/component.h PRE-CREATION > ext/mcpat/cacti/component.cc PRE-CREATION > ext/mcpat/cacti/const.h PRE-CREATION > ext/mcpat/cacti/contention.dat PRE-CREATION > ext/mcpat/cacti/crossbar.h PRE-CREATION > ext/mcpat/cacti/crossbar.cc PRE-CREATION > ext/mcpat/cacti/decoder.h PRE-CREATION > ext/mcpat/cacti/decoder.cc PRE-CREATION > ext/mcpat/cacti/htree2.h PRE-CREATION > ext/mcpat/cacti/htree2.cc PRE-CREATION > ext/mcpat/cacti/io.h PRE-CREATION > ext/mcpat/cacti/io.cc PRE-CREATION > ext/mcpat/cacti/main.cc PRE-CREATION > ext/mcpat/cacti/makefile PRE-CREATION > ext/mcpat/cacti/mat.h PRE-CREATION > ext/mcpat/cacti/mat.cc PRE-CREATION > ext/mcpat/cacti/nuca.h PRE-CREATION > ext/mcpat/cacti/nuca.cc PRE-CREATION > ext/mcpat/cacti/parameter.h PRE-CREATION > ext/mcpat/cacti/parameter.cc PRE-CREATION > ext/mcpat/cacti/router.h PRE-CREATION > ext/mcpat/cacti/router.cc PRE-CREATION > ext/mcpat/cacti/subarray.h PRE-CREATION > ext/mcpat/cacti/subarray.cc PRE-CREATION > ext/mcpat/cacti/technology.cc PRE-CREATION > ext/mcpat/cacti/uca.h PRE-CREATION > ext/mcpat/cacti/uca.cc PRE-CREATION > ext/mcpat/cacti/wire.h PRE-CREATION > ext/mcpat/cacti/wire.cc PRE-CREATION > ext/mcpat/core.h PRE-CREATION > ext/mcpat/core.cc PRE-CREATION > ext/mcpat/globalvar.h PRE-CREATION > ext/mcpat/interconnect.h PRE-CREATION > ext/mcpat/interconnect.cc PRE-CREATION > ext/mcpat/iocontrollers.h PRE-CREATION > ext/mcpat/iocontrollers.cc PRE-CREATION > ext/mcpat/logic.h PRE-CREATION > ext/mcpat/logic.cc PRE-CREATION > ext/mcpat/main.cc PRE-CREATION > ext/mcpat/makefile PRE-CREATION > ext/mcpat/mcpat.mk PRE-CREATION > ext/mcpat/mcpatXeonCore.mk PRE-CREATION > ext/mcpat/memoryctrl.h PRE-CREATION > ext/mcpat/memoryctrl.cc PRE-CREATION > ext/mcpat/noc.h PRE-CREATION > ext/mcpat/noc.cc PRE-CREATION > ext/mcpat/processor.h PRE-CREATION > ext/mcpat/processor.cc PRE-CREATION > ext/mcpat/results/A9_2000 PRE-CREATION > ext/mcpat/results/A9_2000_withIOC PRE-CREATION > ext/mcpat/results/A9_800 PRE-CREATION > ext/mcpat/results/Alpha21364 PRE-CREATION > ext/mcpat/results/Alpha21364_90nm PRE-CREATION > ext/mcpat/results/Penryn PRE-CREATION > ext/mcpat/results/T1 PRE-CREATION > ext/mcpat/results/T1_DC_64 PRE-CREATION > ext/mcpat/results/T1_SBT_64 PRE-CREATION > ext/mcpat/results/T1_ST_64 PRE-CREATION > ext/mcpat/results/T2 PRE-CREATION > ext/mcpat/results/Xeon_core PRE-CREATION > ext/mcpat/results/Xeon_uncore PRE-CREATION > ext/mcpat/sharedcache.h PRE-CREATION > ext/mcpat/sharedcache.cc PRE-CREATION > ext/mcpat/technology_xeon_core.cc PRE-CREATION > ext/mcpat/version.h PRE-CREATION > ext/mcpat/xmlParser.h PRE-CREATION > ext/mcpat/xmlParser.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/2021/diff/ > > > Testing > ------- > > > Thanks, > > Anthony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
