I'm curious how much this matters... are there a significant number of cases right now where we're unnecessarily allocating a data buffer?

My gut reaction is that if we're really looking to defer buffer allocation until we really know whether or not it's necessary, then we should just wait until setData() and just do the allocation then if needed.  (You could also conceivably do the allocation in make*Response(), but I see how that would be complicated by us calling setData() in satisfyCpuSideRequest() before we call make*Response().)  If there are really cases where we currently call setData() but which should be protected by a hasData()/hasRespData() check we could add that inside setData().

src/mem/cache/cache.cc (Diff revision 1)

are there read requests which don't expect data in the response?  unless that's the case, an assert might be more appropriate.

Show all issues

src/mem/cache/cache.cc (Diff revision 1)

is there a case where we reach this code w/o pkt->hasData() being true?  given that 'respond' is only set if we have a modified copy, it seems unlikely... again, if not, perhaps an assert is more apporpriate?

Show all issues

1. Andreas Hansson Dec. 29, 2015, 7:47 a.m. (Dec. 29, 2015, 7:47 a.m.)

   Yes there is. Upgrades in particular.

2. Steve Reinhardt Dec. 29, 2015, 8:34 a.m. (Dec. 29, 2015, 8:34 a.m.)

   So how did this work before?  Were we actually passing data back unnecessarily on upgrades?

3. Andreas Hansson Dec. 29, 2015, 8:36 a.m. (Dec. 29, 2015, 8:36 a.m.)

   Yup

src/mem/cache/mshr.cc (Diff revision 1)

seems complicated to have to track these transitions to know when to call pkt->allocate() and when not to...

Show all issues

1. Andreas Hansson Dec. 29, 2015, 7:47 a.m. (Dec. 29, 2015, 7:47 a.m.)

   This is the only place where we actually change the command for an existing packet. We could make the cmd field const and rather create new packets, but that seems a bit overkill.

2. Steve Reinhardt Dec. 29, 2015, 8:34 a.m. (Dec. 29, 2015, 8:34 a.m.)

   My point was really that there's a lot of embedded/assumed knowledge in this code, i.e., you're putting the call in the first two branches and not the third because you know the result of 'hasData() || hasRespData()' for all six of the commands here (three old & three new) and thus you know in which cases that value is changing and in which cases it is not.  I would feel better if the code didn't rely on all that knowledge.  For example, if pkt->allocate() were idempotent, you could just call it once unconditionally at the end of the function.  I'm not sure it's worth making it idempotent just for that though.
   What do you think of something like this:
   
       // remember whether the current packet has data allocated
   
       bool has_data = pkt->hasData() || pkt->hasRespData();
   
       if (...) {
   
          // current if/else chain
   
       }
   
       // (copy your current comment)
   
       // no need to check pkt->hasData() since the command's already been issued so it's too late to add that
   
       if (!has_data && pkt->hasRespData())
   
           pkt->allocate();
   If you think that's overkill then I won't fight you.  Another alternative would be to leave the code as is, but then add pre/post-condition checks kind of like in my code as assertions (though I'm not sure exactly how that would work).  Seems like it would be useful to have a packet method that checks directly whether or not data has been allocated, i.e., returns flags.isSet(STATIC_DATA|DYNAMIC_DATA).

3. Andreas Hansson Dec. 29, 2015, 8:36 a.m. (Dec. 29, 2015, 8:36 a.m.)

   Sounds like a good idea. I'll change it accordingly.

src/mem/cache/mshr.cc (Diff revision 3)

this comment is no longer needed

Show all issues

src/mem/cache/mshr.cc (Diff revision 3)

is it worth checking pkt->hasData()?  as I said before, I don't see how you could start off with a non-data request and convert it to one that has data, since there's no way to actually set that data field to anything meaningful here.
How about:

    if (!has_data) {

        assert(!pkt->hasData());

        if (pkt->hasRespData()) {

            pkt->allocate();

        }

    }

Show all issues

Ship It!