----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2711/#review6008 -----------------------------------------------------------
Overall, I'm really glad to see this, especially since it doesn't impact performance. Much cleaner! Is there a plan to move the cache_impl.hh code to cache.cc now that it's not templated? src/mem/cache/cache.hh <http://reviews.gem5.org/r/2711/#comment5233> do we still need this? seems like a good opportunity to do s/BlkType/CacheBlk/g src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/2711/#comment5235> should this comment be above the method? src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/2711/#comment5234> move open brace up to same line as 'for' src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/2711/#comment5236> is 'this->' necessary? src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/2711/#comment5237> '{' placement src/mem/cache/tags/base_set_assoc.cc <http://reviews.gem5.org/r/2711/#comment5239> do we really want to replace the visitor pattern with this? I don't know how often this is used, but building this whole list seems like unnecessary overhead. I guess what we really need is the C++ equivalent of python generators... src/mem/cache/tags/fa_lru.cc <http://reviews.gem5.org/r/2711/#comment5238> space before '{' - Steve Reinhardt On March 30, 2015, 2:16 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2711/ > ----------------------------------------------------------- > > (Updated March 30, 2015, 2:16 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10777:9f23af363f48 > --------------------------- > mem: Remove templates in cache model > > This patch changes the cache implementation to rely on virtual methods > rather than using the replacement policy as a template argument. > > There is no impact on the simulation performance, and overall the > changes make it easier to modify (and subclass) the cache and/or > replacement policy. > > > Diffs > ----- > > src/mem/cache/base.cc 8a7285d6197e > src/mem/cache/blk.hh 8a7285d6197e > src/mem/cache/cache.hh 8a7285d6197e > src/mem/cache/cache.cc 8a7285d6197e > src/mem/cache/cache_impl.hh 8a7285d6197e > src/mem/cache/tags/base.hh 8a7285d6197e > src/mem/cache/tags/base_set_assoc.hh 8a7285d6197e > src/mem/cache/tags/base_set_assoc.cc 8a7285d6197e > src/mem/cache/tags/fa_lru.hh 8a7285d6197e > src/mem/cache/tags/lru.hh 8a7285d6197e > src/mem/cache/tags/lru.cc 8a7285d6197e > src/mem/cache/tags/fa_lru.cc 8a7285d6197e > src/mem/cache/tags/random_repl.cc 8a7285d6197e > src/mem/cache/tags/random_repl.hh 8a7285d6197e > > Diff: http://reviews.gem5.org/r/2711/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
