----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2167/#review5191 -----------------------------------------------------------
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. - Andreas Sandberg On June 25, 2014, 1:26 a.m., Anthony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2167/ > ----------------------------------------------------------- > > (Updated June 25, 2014, 1:26 a.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
