----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2981/#review6961 -----------------------------------------------------------
This is a lot better. Thanks for these changes. I agree that we need to broadcast this change loudly since it will result in silent performance bugs in non-mainline protocols. Can you please comment on the testing you did to ensure performance correctness of mainline protocols? src/mem/ruby/system/Sequencer.cc <http://reviews.gem5.org/r/2981/#comment5983> Could you please add a warn_once() here that states "Replacement policy updates recently became the responsibility of SLICC state machines. Make sure to setMRU() near callbacks in .sm files!"? src/mem/ruby/system/Sequencer.cc <http://reviews.gem5.org/r/2981/#comment5982> Can m_instCache_ptr be removed now? Doesn't look like it's being used elsewhere. - Joel Hestness On Aug. 2, 2015, 5:42 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2981/ > ----------------------------------------------------------- > > (Updated Aug. 2, 2015, 5:42 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11002:86776d8b9a7d > --------------------------- > ruby: call setMRU from L1 controllers, not from sequencer > Currently the sequencer calls the function setMRU that updates the replacement > policy structures with the first level caches. While functionally this is > correct, the problem is that this requires calling findTagInSet() which is an > expensive function. This patch removes the calls to setMRU from the > sequencer. > All controllers should now update the replacement policy on their own. > > The set and the way index for a given cache entry can be found within the > AbstractCacheEntry structure. Use these indicies to update the replacement > policy structures. > > > Diffs > ----- > > src/mem/protocol/MESI_Three_Level-L0cache.sm a618349a7953 > src/mem/protocol/MESI_Two_Level-L1cache.sm a618349a7953 > src/mem/protocol/MI_example-cache.sm a618349a7953 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm a618349a7953 > src/mem/protocol/MOESI_CMP_token-L1cache.sm a618349a7953 > src/mem/protocol/MOESI_hammer-cache.sm a618349a7953 > src/mem/protocol/RubySlicc_Types.sm a618349a7953 > src/mem/ruby/structures/CacheMemory.hh a618349a7953 > src/mem/ruby/structures/CacheMemory.cc a618349a7953 > src/mem/ruby/system/Sequencer.cc a618349a7953 > > Diff: http://reviews.gem5.org/r/2981/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
