> 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. > > Nathan Binkert wrote: > 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.
I"ll see what I can do. But I don't particularly like the idea of an idexing function, I prefer forms 2/3. > 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. > > Nathan Binkert wrote: > 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. ah, i see. you got me. that is a problem. > 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. > > Nathan Binkert wrote: > 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. i dunno...you want overload the meaning of tid in a Request for MemCmd::Writebacks? I am totally all right with hacky overloads when things are in my own tree but this seems trickier to enforce/minimize confusion about. > 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. > > Nathan Binkert wrote: > Is is that hard? I don't know. Is it? I haven't opened a file in dev/ in years. Does this involve changing all device models? Changing something in a port interface? Which? I don't know. I have not looked into it at all. I tried to page out as little of what I was doing to fix this vector length matching thing, and I don't want to delay getting back to other crap that I have to do. I consider this a difference between "should" and "must". I consider the vector thing a "must" - I introduced the code that broke it and it currently causes problems. This is definitely a "should", IMHO. I've got to get to Crossfit now, I'm going to be late! - 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
