----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1931/#review4455 -----------------------------------------------------------
In general this looks like a positive direction, but I'm concerned about keeping the python tag class and the cache template instance in sync. For example, it looks to me like if I specify a fully associative cache (via the cache's assoc parameter), but pass in an LRU tag SimObject, then the dynamic_cast in the tag initializer of the Cache constructor will fail, and I'll get a segfault in the constructor at 'tags->setCache(this);'. Not very user-friendly at best! One possibility would be to use dynamic_casts in BaseCacheParams::create() to figure out the type of the tags object and use the right Cache template instance accordingly. You lose the magic of letting the system pick the best tag instance for you (in the case where that makes sense) but at least you avoid the potential for a mismatch between the two types. Another possibility is to change the python perspective from there being two independent classes (Cache and LRU/FALRU) to an inheritance model, where you instantiate LRUCache or FALRUCache in python, and those map to Cache<LRU> or Cache<FALRU> in the C++. Then the python tag params become additional params on the derived Cache object. src/mem/cache/tags/lru.hh <http://reviews.gem5.org/r/1931/#comment4178> I'm guessing this violates Nate's include-sorting rules... - Steve Reinhardt On June 20, 2013, 5:47 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1931/ > ----------------------------------------------------------- > > (Updated June 20, 2013, 5:47 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 9791:0946f340b959 > --------------------------- > mem: Reorganize cache tags and make them a SimObject > > This patch reorganizes the cache tags to allow more flexibility to > implement new replacement policies. The base tags class is now a > clocked object so that derived classes can use a clock if they need > one. Also having deriving from SimObject allows specialized Tag > classes to be swapped in/out in .py files. > > The cache set is now templatized to allow it to contain customized > cache blocks with additional informaiton. This involved moving code to > the .hh file and removing cacheset.cc. > > The statistics belonging to the cache tags are now including ".tags" > in their name. Hence, the stats need an update to reflect the change > in naming. > > Note that this follows on: http://reviews.gem5.org/r/1875/ > > > Diffs > ----- > > src/mem/cache/tags/lru.cc ff17ab994003 > src/mem/cache/tags/lru.hh ff17ab994003 > src/mem/cache/tags/fa_lru.cc ff17ab994003 > src/mem/cache/tags/fa_lru.hh ff17ab994003 > src/mem/cache/tags/cacheset.cc ff17ab994003 > src/mem/cache/tags/cacheset.hh ff17ab994003 > src/mem/cache/tags/base.cc ff17ab994003 > src/mem/cache/tags/base.hh ff17ab994003 > src/mem/cache/tags/Tags.py PRE-CREATION > src/mem/cache/tags/SConscript ff17ab994003 > src/mem/cache/cache_impl.hh ff17ab994003 > src/mem/cache/cache.hh ff17ab994003 > src/mem/cache/base.cc ff17ab994003 > src/mem/cache/BaseCache.py ff17ab994003 > > Diff: http://reviews.gem5.org/r/1931/diff/ > > > Testing > ------- > > After some sedding of the stats (for tag stats include .tags in the name) all > regressions pass > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
