----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2167/#review5214 -----------------------------------------------------------
Hi Tony, Sorry for taking so long to get around to this. I think your patch is definitely a step in the right direction. My main concern is that it doesn't go far enough, in the sense that there's still a lot of duplication between the LRU and PseudoLRU classes that could be factored out into the common base class. I'm just eyeballing the diff here, so I might be missing something, but the two versions of insertBlock() and invalidate() look the same to me, and accessBlock() looks the same except for the added moveToHead() line in LRU (and the associated DPRINTF). Also, is there a need for all these functions to be virtual? The way we have the Cache class templated on the tag class, I would think not. (The whole intent of that templating is to avoid those virtual function calls.) If you don't want to add an additional virtual function dispatch, you can continue to have the most derived classes implement the interface, but then have them call non-virtual, potentially inlined common methods in the base class that provide the common code; or you can use CRTP to include code from the derived classes in what are syntactically base class methods. If you don't like those solutions, I'd be willing to consider trading a virtual function dispatch for less code redundancy. (Others can chime in on where they stand on these tradeoffs.) Also, it seems like preferentially replacing invalid blocks is orthogonal to the LRU vs. PseudoLRU replacement policy, and it would be nice to make it so. I'd be OK with breaking backward compatibility by making it universal and putting it in the base class too. - Steve Reinhardt On June 24, 2014, 4:26 p.m., Anthony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2167/ > ----------------------------------------------------------- > > (Updated June 24, 2014, 4:26 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10243:8f4099f6b9a2 > --------------------------- > mem: re-factor LRU code and add random replacement cache tags > > this patch implements a new tags class that uses a random replacement policy. > these tags prefer to evict invalid blocks first, if none are available a > replacement candidate is chosen at random. > > this patch factors out the common code in the LRU class and creates a new > abstract class: the BaseLRU class. any LRU tag or derivative of LRU must > implement the methods related to the actual replacement policy. these are the > following methods, which are pure virtual methods in BaseLRU: > > accessBlock() > findVictim() > insertBlock() > invalidate() > > > Diffs > ----- > > src/mem/cache/base.cc cb4e86c177672fde6be7a409793c944e36353fc0 > src/mem/cache/cache.cc cb4e86c177672fde6be7a409793c944e36353fc0 > src/mem/cache/tags/SConscript cb4e86c177672fde6be7a409793c944e36353fc0 > src/mem/cache/tags/Tags.py cb4e86c177672fde6be7a409793c944e36353fc0 > src/mem/cache/tags/base_lru.hh PRE-CREATION > src/mem/cache/tags/base_lru.cc PRE-CREATION > src/mem/cache/tags/lru.hh cb4e86c177672fde6be7a409793c944e36353fc0 > src/mem/cache/tags/lru.cc cb4e86c177672fde6be7a409793c944e36353fc0 > src/mem/cache/tags/pseudo_lru.hh PRE-CREATION > src/mem/cache/tags/pseudo_lru.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/2167/diff/ > > > Testing > ------- > > Regressions pass > > > Thanks, > > Anthony Gutierrez > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
