Cache: fix vector stats in classic cache to have matching lengths

Information
Submitter:	Lisa Hsu
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

Cache: fix vector stats in classic cache to have matching lengths

Testing Done

src/mem/cache/base.hh (Diff revision 1)

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 April 22, 2011, 12:26 p.m. (April 22, 2011, 12:26 p.m.)

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 April 22, 2011, 1:44 p.m. (April 22, 2011, 1:44 p.m.)

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.

Lisa Hsu April 23, 2011, 2:43 a.m. (April 23, 2011, 2:43 a.m.)

I"ll see what I can do.  But I don't particularly like the idea of an idexing function, I prefer forms 2/3.

src/mem/cache/base.hh (Diff revision 1)

_numSharingContexts has the + 1 in it for devices.  Is that a problem?  Seems like you need a parameter to know that.

Lisa Hsu April 22, 2011, 12:11 p.m. (April 22, 2011, 12:11 p.m.)

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 April 22, 2011, 1:49 p.m. (April 22, 2011, 1:49 p.m.)

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.

Lisa Hsu April 23, 2011, 2:43 a.m. (April 23, 2011, 2:43 a.m.)
```
ah, i see.  you got me.  that is a problem.
```

src/mem/cache/base.hh (Diff revision 1)

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 April 22, 2011, 12:15 p.m. (April 22, 2011, 12:15 p.m.)

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.

Nathan Binkert April 22, 2011, 1:50 p.m. (April 22, 2011, 1:50 p.m.)

It would be ideal if we always explicitly set the ID on every request.  That way we don't get random wrong answers.

src/mem/cache/cache_impl.hh (Diff revision 1)

This makes me think that some sort of index function is the right way to go.

Lisa Hsu April 22, 2011, 12:16 p.m. (April 22, 2011, 12:16 p.m.)

I can mod the macros to take ids instead of pkt to encompass these cases as well.

src/mem/cache/cache_impl.hh (Diff revision 1)

Is it not possible to figure out which thread caused the wb?

Lisa Hsu April 22, 2011, 12:18 p.m. (April 22, 2011, 12:18 p.m.)

this isn't recording who caused the wb, this is recording who is getting written back.

Nathan Binkert April 22, 2011, 1:51 p.m. (April 22, 2011, 1:51 p.m.)

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.

Lisa Hsu April 23, 2011, 2:47 a.m. (April 23, 2011, 2:47 a.m.)

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.

src/mem/cache/cache_impl.hh (Diff revision 1)

I think we should force context IDs to be set even by devices.

Lisa Hsu April 22, 2011, 12:19 p.m. (April 22, 2011, 12:19 p.m.)
```
not it.
```
Nathan Binkert April 22, 2011, 1:52 p.m. (April 22, 2011, 1:52 p.m.)
```
Is is that hard?
```

Lisa Hsu April 23, 2011, 2:52 a.m. (April 23, 2011, 2:52 a.m.)

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!

src/mem/cache/cache_impl.hh (Diff revision 1)

Index function.

Lisa Hsu April 22, 2011, 12:20 p.m. (April 22, 2011, 12:20 p.m.)

oops - missed this one.  should have been converted to the macro.

You have a pending review.

Review Board 2.0.15

Screenshots

Files