> On July 20, 2014, 11:10 p.m., Steve Reinhardt wrote: > > 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. > > > >
I got rid of the virtual functions and have consolidated more of the code into the base class. I also changed the names of the base and random classes. - Anthony ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2167/#review5214 ----------------------------------------------------------- On July 21, 2014, 6:45 p.m., Anthony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2167/ > ----------------------------------------------------------- > > (Updated July 21, 2014, 6:45 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > 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 BaseSetAssoc class. any set associative tag class must > implement the functionality related to the actual replacement policy in the > following methods: > > accessBlock() > findVictim() > insertBlock() > invalidate() > > > Diffs > ----- > > src/mem/cache/base.cc 23384aa97d8533f6e3f812d015dccaab3c0267af > src/mem/cache/cache.cc 23384aa97d8533f6e3f812d015dccaab3c0267af > src/mem/cache/tags/SConscript 23384aa97d8533f6e3f812d015dccaab3c0267af > src/mem/cache/tags/Tags.py 23384aa97d8533f6e3f812d015dccaab3c0267af > src/mem/cache/tags/base_set_assoc.hh PRE-CREATION > src/mem/cache/tags/base_set_assoc.cc PRE-CREATION > src/mem/cache/tags/lru.hh 23384aa97d8533f6e3f812d015dccaab3c0267af > src/mem/cache/tags/lru.cc 23384aa97d8533f6e3f812d015dccaab3c0267af > src/mem/cache/tags/random_repl.hh PRE-CREATION > src/mem/cache/tags/random_repl.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
