> On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/base.hh, line 505 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11759#file11759line505> > > > > _numSharingContexts has the + 1 in it for devices. Is that a problem? > > Seems like you need a parameter to know that.
I have a comment in the patch that explains it. It has nothing to do with devices and everything to do with switching CPUs. if you have switch CPUs, you will have N*2 contexts and throughout the course of simulation you'll mostly care about [N, 2N). Hence, the mod. > On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/base.hh, line 507 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11759#file11759line507> > > > > Maybe you can use a constant to indicate the -1 is a device. Also, > > let's make sure that you initialize to something other than -1. Why? Devices never set the id so this assertion confirms that the id is never set. It's more that if it has come from a device, something had better not have set the contextId, otherwise something is broken. > On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/cache_impl.hh, line 1013 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11762#file11762line1013> > > > > This makes me think that some sort of index function is the right way > > to go. I can mod the macros to take ids instead of pkt to encompass these cases as well. > On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/cache_impl.hh, line 1021 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11762#file11762line1021> > > > > Is it not possible to figure out which thread caused the wb? this isn't recording who caused the wb, this is recording who is getting written back. > On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/cache_impl.hh, line 1087 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11762#file11762line1087> > > > > I think we should force context IDs to be set even by devices. not it. > On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/cache_impl.hh, line 1443 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11762#file11762line1443> > > > > Index function. oops - missed this one. should have been converted to the macro. > On 2011-04-22 18:51:32, Nathan Binkert wrote: > > src/mem/cache/base.hh, line 503 > > <http://reviews.m5sim.org/r/654/diff/1/?file=11759#file11759line503> > > > > This can pretty easily be done with a template: > > > > template <class STAT> > > void > > incThreadVectorStat(PacketPtr pkt, STAT &stat) > > { > > } > > > > I'm pretty sure that you could even do this: > > > > template <class STAT> > > STAT::Proxy & > > getThreadVectorStat(PacketPtr pkt, STAT &stat[]) > > { > > if (pkt->req->hasContextId()) { > > return stat[pkt->cmdToIndex()][pkt->req->contextId() % > > _numSharingContexts]; > > } else { > > assert(pkt->req->contextId() == -1); > > assert(FULL_SYSTEM); > > return stat[pkt->cmdToIndex()][_numSharingContexts]; > > } > > > > And do this: > > getThreadVectorStat(pkt, misses)++; > > getThreadVectorStat(pkt, misses) += 1; > > > > Actually, it may just be cleaner to have a function that returns the > > index. Then you don't need all of the template mubmo jumbo. I did a single pass of trying to pass a reference of a stat to a function. When it didn't work immediately, I went to macro. I have enough other stuff to do that that level of beauty for a dying memory model is kinda irrelevant to me right now, and it's not clear to me that it's more beautiful either. Something returning the index seems messier to me. Then you'd need: int index = getIndex(pkt); statName[pkt->cmdToIndex()][index]++; everywhere, instead of: incrementStat(pkt, statName); or incrementStat(pkt->req->contextId(), statName); I think the latter two are prettier than the first. I might switch to the 3rd before I push. - Lisa ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/654/#review1142 ----------------------------------------------------------- On 2011-04-22 17:16:05, Lisa Hsu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/654/ > ----------------------------------------------------------- > > (Updated 2011-04-22 17:16:05) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > Cache: fix vector stats in classic cache to have matching lengths > > > Diffs > ----- > > configs/common/CacheConfig.py 914389024c33 > src/cpu/base.cc 914389024c33 > src/mem/cache/BaseCache.py 914389024c33 > src/mem/cache/base.hh 914389024c33 > src/mem/cache/base.cc 914389024c33 > src/mem/cache/blk.hh 914389024c33 > src/mem/cache/cache_impl.hh 914389024c33 > src/mem/cache/tags/base.cc 914389024c33 > src/mem/cache/tags/lru.cc 914389024c33 > > Diff: http://reviews.m5sim.org/r/654/diff > > > Testing > ------- > > > Thanks, > > Lisa > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
