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
