Re: [gem5-dev] Review Request 2167: mem: re-factor LRU code and add random replacement cache tags
--- 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
Re: [gem5-dev] Review Request 2167: mem: re-factor LRU code and add random replacement cache tags
--- 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
Re: [gem5-dev] Review Request 2167: mem: re-factor LRU code and add random replacement cache tags
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
