> 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. > > Andreas Hansson wrote: > 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.)? > > > > > > Joel Hestness wrote: > The version of McPAT to be added in this patch doesn't support either > frequency or voltage scaling, but it can be used to model power/energy over > regions of execution (simulated in gem5) during which frequency or voltage do > not change. Yasuko's and my changes in http://reviews.gem5.org/r/2117/ take a > substantial step toward a more modular component interface that will make it > much easier to implement frequency scaling. The changes introduced in McPAT > 1.0 enable many of the pieces required for voltage scaling. There is a fair > amount of work to enabling McPAT to model multiple independently changing > clock/frequency domains with arbitrary subcomponents, but there is > substantial value in being able to use McPAT as included in this request.
So, is there a consensus here? Should I go ahead and get this patch shipped? - Anthony ----------------------------------------------------------- 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
