----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1294/#review3326 -----------------------------------------------------------
Hi Lena, This is great, thanks! Pretty much exactly what I was envisioning. I've got a handful of very minor things below; I expect you're on your way back to Wisconsin, so if you won't be able to get to them soon, let us know and perhaps someone else can take care of them so we can get this committed (assuming you don't have any objections to what I'm suggesting). Also, I had one more thought: you've added a number of 'assert(blk != tempBlk);' lines, which is a good idea, but it would be nicer if we could get the same effect without the additional clutter. One possibility would be to set the 'set' field of tempBlk to -1, then add an assert to check for that in LRU::invalidate(). I'd be interested in what others think of that. src/mem/cache/blk.hh <http://reviews.gem5.org/r/1294/#comment3417> isTouched is a bool, so rhs is preferably 'false' rather than '0' src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1294/#comment3420> Note that we already have assert(blk && blk->isValid()); at the top of the function, so assert(blk) is redundant here. src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1294/#comment3419> See previous comment about assert(blk) src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1294/#comment3423> I expect this could be folded with the enclosing if (blk), i.e., just make the outer condition if (blk && blk->isValid()) { src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1294/#comment3418> I see what you did here... since tempBlk was previously invalidated, re-invalidating it should be unnecessary. That seems like a fine optimization, but I think the comment should also be updated to better reflect what's going on here. src/mem/cache/cache_impl.hh <http://reviews.gem5.org/r/1294/#comment3421> Note there's an early exit from the function above if blk is NULL, so this assert is also unnecessary. src/mem/cache/tags/lru.cc <http://reviews.gem5.org/r/1294/#comment3422> why did we lose the space here? seems like a typo - Steve Reinhardt On Aug. 24, 2012, 7:31 p.m., Lena Olson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1294/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2012, 7:31 p.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9167:1bd0c85445a2 > --------------------------- > Cache: Split invalidateBlk up to seperate block vs. tags > > This seperates the functionality to clear the state in a block into blk.hh and > the functionality to udpate the tag information into the tags. This gets rid > of > the case where calling invalidateBlk on an already-invalid block does > something > different than calling it on a valid block, which was confusing. > > > Diffs > ----- > > src/mem/cache/blk.hh 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/cache_impl.hh 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/tags/fa_lru.hh 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/tags/fa_lru.cc 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/tags/iic.hh 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/tags/iic.cc 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/tags/lru.hh 1d983855df2c941655cda665056c6b3a8bd3d463 > src/mem/cache/tags/lru.cc 1d983855df2c941655cda665056c6b3a8bd3d463 > > Diff: http://reviews.gem5.org/r/1294/diff/ > > > Testing > ------- > > > Thanks, > > Lena Olson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
