(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
