> On April 1, 2015, 6:30 p.m., Steve Reinhardt wrote: > > src/mem/cache/tags/base_set_assoc.cc, line 202 > > <http://reviews.gem5.org/r/2711/diff/1/?file=44369#file44369line202> > > > > 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... > > Andreas Hansson wrote: > It is only used as part of flushing and checkpointing so it should not be > a performance issue. Perhaps leave it as is for now? > > Steve Reinhardt wrote: > Perhaps... if we were just implementing this for the first time, it would > only be a little sad, but the fact that we had a nice callback structure and > are replacing it with this inefficient approach makes me extra sad, and the > fact that we're still layering a callback interface on top of it just deepens > the sorrow :(. > > Wouldn't it be as simple as replacing BaseTags::getBlkList() with > virtual bool visitBlocks(Cache *, VisitorFnPtr fn) = 0; > > then rewrite this function: > > bool > Cache::visitBlocks(VisitorFnPtr fn) > { > return tags->visitBlocks(this, fn); > } > > and for example: > > bool > BaseSetAssoc::visitBlocks(Cache *cache, VisitorFnPtr fn) > { > for (...) { > for (...) { > if (!(cache->*fn)(sets[i].blks[j])) > return false; > } > } > > return true; > } > > Steve Reinhardt wrote: > Sorry about the line wrapping... I hypothesize that I should have > un-checked the 'enable markdown' box on that one. Let's see if that helps: > > > bool > BaseSetAssoc::visitBlocks(Cache *cache, VisitorFnPtr fn) > { > for (...) { > for (...) { > if (!(cache->*fn)(sets[i].blks[j])) > return false; > } > } > > return true; > }
Good idea, I struggle to make this work though, since there is no way of making the tags include cache.hh (without things being circular). The only sensible suggestion seems to be a BaseCacheBlkVisitor with a virtual bool operator()(BlkType& blk) =0 that we then expose to the tags visitBlocks, and subclass for the various visitors in the cache. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2711/#review6008 ----------------------------------------------------------- On April 2, 2015, 9:09 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2711/ > ----------------------------------------------------------- > > (Updated April 2, 2015, 9:09 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10777:7a17ca9141fb > --------------------------- > 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/fa_lru.cc 8a7285d6197e > src/mem/cache/tags/lru.hh 8a7285d6197e > src/mem/cache/tags/lru.cc 8a7285d6197e > src/mem/cache/tags/random_repl.hh 8a7285d6197e > src/mem/cache/tags/random_repl.cc 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
