That seems like a good plan to me -- the current way is non-intuitive and kind of kludgy (which is my excuse for missing the case of the temporary block in my initial patch =) ), and separating out the the management of the cache block from management of the cache tags makes much more sense to me. Give me a couple days and I can submit a patch that does that.
2012/7/8 Ali Saidi <[email protected]>: > Sounds good to me…. Wanna split them up? > > Ali > > > On Jul 6, 2012, at 9:51 AM, Steve Reinhardt wrote: > >> (I started to write this as a reviewboard comment, but it got kind of >> long...) >> >> I can see how this might work, but it seems a little obscure. In >> particular, it's not obvious that checking blk->isValid() is equivalent to >> checking for the temp block. The fact that it works is due to a recent >> hack that I didn't notice until now ( >> http://repo.gem5.org/gem5/rev/08cc303b718b) which I actually don't like... >> I guess I wasn't paying attention when Ali committed that. My first >> thought on seeing: >> >> blk->status &= ~BlkValid; >> tags->invalidateBlk(blk); >> >> is why in the world would we redundantly mark the block invalid twice, and >> why don't we get rid of that first line? You have to track down the >> changeset to understand why this line was added. (We still do that too >> often: too much documentation is put in the commit messages rather than in >> comments in the code.) >> >> Anyway, I think the fundamental problem here is that we are combining two >> things in the same function: >> 1. Resetting the state of the CacheBlk class >> 2. Doing the associated tracking of statistics etc. which are associated >> with the TagStore class >> >> Usually this works OK, but in the case of tempBlock, it's not tracked by >> the tags, so we only want to do #1 and not #2. My point above is that >> implicitly controlling this by way of the valid bit seems confusing and >> error-prone to me. In addition, pushing #1 into the tags probably means >> that anything other than LRU doesn't work anymore (not that I care much >> about IIC, but it would be nice if FALRU didn't wither away, and it's nice >> to enable other plug-in replacement policies). >> >> I'll suggest that we really need two functions, one that's a method on >> CacheBlk to do #1, and another that replaces the existing method on >> TagStore to do #2. Then we call both of these in most places where we now >> call tags->invalidateBlk(), but in the tempBlk case we only call the >> former. For convenience, we could define a method on Cache like this: >> >> void >> Cache<TagStore>::invalidateBlk(BlkType *blk) >> { >> if (blk == NULL) >> return; >> blk->invalidate(); >> if (blk != tempBlock) >> tags->invalidateBlk(blk); >> } >> >> and just drop it in in place of tags->invalidateBlk(). This would even let >> us get rid of the conditional in a couple of places where we put >> tags->invalidateBlk() inside an if (blk != NULL). I think we can trust the >> optimizer to get rid of the check when it's not needed. >> >> Thoughts? >> >> In the long run, it would be nice to make all (or at least most) of the >> data members of CacheBlk private, and provide methods to do the needed >> state transitions like invalidate(), markWritable(), setDirty(), >> clearDirty(), etc. For example, right now there's one spot where we just >> set the status to 0: >> http://repo.gem5.org/gem5/annotate/aa8fd8f6a495/src/mem/cache/cache_impl.hh#l1061 >> and it's not clear whether we should be calling invalidate() at that point >> or not. >> >> Steve >> _______________________________________________ >> gem5-dev mailing list >> [email protected] >> http://m5sim.org/mailman/listinfo/gem5-dev >> > > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
