----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2981/#review6870 -----------------------------------------------------------
I'm still pretty ambivalent about this change. There are now 4 potential ways to update MRU: (1) direct call from an action to a cache's setMRU, (2)/(3) calling an action from a transition that updates a specific cache's MRU, and (4) calling an action from a transition that updates the MRU of all caches. This seems quite fragile and could result in all kinds of very difficult-to-debug performance problems. I feel that we should modify some more with the aim of keeping replacement policy updates within actions that call hit callbacks. This might mean adding actions that are specific to I- vs. D-caches, but it would limit the number of calls to CacheMemory::setMRU() as well as the number of potentially sneaky performance bugs. src/mem/protocol/MESI_Three_Level-L0cache.sm (line 512) <http://reviews.gem5.org/r/2981/#comment5938> Can you consistently capitalize 'MRU' in these? src/mem/protocol/MESI_Three_Level-L0cache.sm (line 517) <http://reviews.gem5.org/r/2981/#comment5939> It would be helpful if these names were a little more verbose. Perhaps sm_setBothCacheMRU, uic_setICacheMRU and udc_setDCacheMRU? Maybe even use more consistent action ID tags? This comment applies to all modified protocols. src/mem/protocol/MOESI_CMP_token-L1cache.sm (line 1287) <http://reviews.gem5.org/r/2981/#comment5940> In this file, it appears that sm_setMruCache is only ever called from before x_external_load_hit and xx_external_store_hit. Can we just move the MRU updates into those actions instead? - Joel Hestness On July 19, 2015, 11:47 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2981/ > ----------------------------------------------------------- > > (Updated July 19, 2015, 11:47 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10937:8903e39b8b12 > --------------------------- > 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/ruby/structures/CacheMemory.hh 3a925f9856b1 > src/mem/ruby/structures/CacheMemory.cc 3a925f9856b1 > src/mem/ruby/system/Sequencer.cc 3a925f9856b1 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm 3a925f9856b1 > src/mem/protocol/MOESI_CMP_token-L1cache.sm 3a925f9856b1 > src/mem/protocol/MOESI_hammer-cache.sm 3a925f9856b1 > src/mem/protocol/RubySlicc_Types.sm 3a925f9856b1 > src/mem/protocol/MI_example-cache.sm 3a925f9856b1 > src/mem/protocol/MESI_Two_Level-L1cache.sm 3a925f9856b1 > src/mem/protocol/MESI_Three_Level-L0cache.sm 3a925f9856b1 > > 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
