> On Dec. 23, 2015, 4:09 p.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 2060 > > <http://reviews.gem5.org/r/3259/diff/1/?file=52246#file52246line2060> > > > > 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? > > Andreas Hansson wrote: > Yes there is. Upgrades in particular.
So how did this work before? Were we actually passing data back unnecessarily on upgrades? > On Dec. 23, 2015, 4:09 p.m., Steve Reinhardt wrote: > > src/mem/cache/mshr.cc, line 123 > > <http://reviews.gem5.org/r/3259/diff/1/?file=52247#file52247line123> > > > > seems complicated to have to track these transitions to know when to > > call pkt->allocate() and when not to... > > Andreas Hansson wrote: > 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. 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). - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3259/#review7741 ----------------------------------------------------------- On Dec. 29, 2015, 7:47 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3259/ > ----------------------------------------------------------- > > (Updated Dec. 29, 2015, 7:47 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11283:7bd963b6647d > --------------------------- > mem: Do not allocate space for packet data if not needed > > This patch looks at the request and response command to determine if > either actually has any data payload, and if not, we do not allocate > any space for packet data. > > The only tricky case is where the command type is changed as part of > the MSHR functionality. In these cases where the original packet had > no data, but the new packet does, we need to explicitly call > allocate(). > > > Diffs > ----- > > src/mem/packet.hh 3fd1142adad9 > src/mem/cache/cache.cc 3fd1142adad9 > src/mem/cache/mshr.cc 3fd1142adad9 > > Diff: http://reviews.gem5.org/r/3259/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
