> On Dec. 24, 2015, 12:09 a.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. > > Steve Reinhardt wrote: > So how did this work before? Were we actually passing data back > unnecessarily on upgrades?
Yup > On Dec. 24, 2015, 12:09 a.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. > > Steve Reinhardt wrote: > 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). Sounds like a good idea. I'll change it accordingly. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3259/#review7741 ----------------------------------------------------------- On Dec. 29, 2015, 3:47 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3259/ > ----------------------------------------------------------- > > (Updated Dec. 29, 2015, 3:47 p.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
