> On July 15, 2014, 2:47 p.m., Andreas Sandberg wrote: > > Overall, I think this patch looks good and the refactoring is a well-needed > > change to the way we implement replacement policies. > > > > At a high level, I'd really appreciate it if you could split this patch > > into two patches so the refactoring becomes its own commit. That makes > > things like bisection much cleaner in the future. > > > > I also found the naming of the different classes a bit confusing. I would > > suggest that you rename BaseLRU -> BaseSetAssoc or something similar. After > > all, the common stuff that goes into this base class isn't really LRU > > specific. I also get a bit confused by the PseudoLRU class. When I think of > > pseudo-LRU, I generally think of things like tree-based LRU or the > > algorithm used in Nehalem. I'd suggest that you rename it to something with > > random in the name instead.
I think the benefit of splitting it vs the effort to do so is too great. However the naming change would be useful. Thanks a lot for the patch Tony. - Ali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2167/#review5191 ----------------------------------------------------------- On June 24, 2014, 11: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, 11: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
