> 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. > > Lisa Hsu wrote: > 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.
It's not that macros are ugly, but they tend to be much more difficult to debug and make code more brittle. A macro in a .cc file is one thing, but in a header, we should really try to avoid it if at all possible. And just because some day, hypothetically in the future, code may be thrown away doesn't mean that we should allow it to become more brittle. > 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. > > Lisa Hsu wrote: > 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. I understand that, but if _numSharingContexts includes the device (which it does if your in FULL_SYSTEM mode, when you do the mod, you'll be off by one. (At least I'm pretty sure that's true.) Think of this. We have 2 cores and 1 device. if you're talking to the last core which is core 3 (2N - 1) Then you're going to do 3 % 3 and you're going index to zero instead of one. > 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. > > Lisa Hsu wrote: > 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. It would be ideal if we always explicitly set the ID on every request. That way we don't get random wrong answers. > 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? > > Lisa Hsu wrote: > this isn't recording who caused the wb, this is recording who is getting > written back. Right, but if we don't know the tid, is there something tid related that we do know that could be interesting? That's why I was suggesting using the thread that caused the writeback as the tid in this case. > 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. > > Lisa Hsu wrote: > not it. Is is that hard? - Nathan ----------------------------------------------------------- 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
