> On March 4, 2016, 9:13 a.m., Steve Reinhardt wrote:
> > Overall I think this specialization is good. I think the tuple return
> > thing is very clunky though. It looks like getNextQueueEntry() is only
> > called from getTimingPacket(), which in turn is only called from
> > sendDeferredPacket(). Also a lot of getTimingPacket() is basically
> >
> > if (mshr) {
> > // do a bunch of stuff
> > } else {
> > assert(wq_entry);
> > // do a bunch of other stuff
> > }
> >
> > So I think this whole call stack structure really only made sense because
> > of the unification of the write buffer and MSHR objects, and now it no
> > longer makes sense, and the tuple return values are a symptom of that.
> >
> > I think if we're going to bother to do this specialization, we really need
> > to refactor this piece of code avoid this pointless merging and forking of
> > the MSHR vs write buffer code paths that leads to these tuple return
> > values. In cases where the code paths are merged and we still want to do
> > something different between the two cases, we should try and do that by
> > defining appropriate virtual functions on QueueEntry, and perhaps (if
> > necessary) creating a common base class for the Queue<> classes and
> > defining some virtual functions on that.
>
> Andreas Hansson wrote:
> I am also not a huge fan of the tuple approach, but the only sensible
> solution would be to avoid calling the method, and instead merging these
> chains.
>
> I really do not like the idea of making the queue entry clever. I'd
> prefer if both the write queue entry and MSHR was as close to a struct as
> possible. The code is already challenging to follow, and splitting
> functionality into cache, MSHR and write queue entry is a recipee for
> disaster imho.
>
> Any other ideas of how to tidy this up?
>
> Andreas Hansson wrote:
> I am keen to get this patch finished up and pushed as there are follow-on
> patches depending on it, including a number of simplifications.
>
> Steve, are you happy with it as is?
Sorry for the delayed response... I've been meaning to take a closer look and
provide a more concrete suggestion but I was traveling and didn't have a chance
until just now.
On closer inspection, the current structure really is pointless.
getTimingPacket() is really two different functions now split by the if (mshr)
{ ... } else { assert(wq_entry); ... }. I'm sure there are multiple ways this
could be cleaned up, but here's one that comes to mind:
- Take what's inside the 'if (mshr) { ... }' part of getTimingPacket and make a
new function out of it:
PacketPtr makePacketFromMSHR(MSHR *) { ... }
- Take what's inside the 'else { assert(wq_entry) ... }' part of
getTimingPacket and make a new function out of that:
PacketPtr makePacketFromWriteBuffer(WriteQueueEntry *) { ... }
- Change the signature of getNextQueueEntry to return a PacketPtr (might even
want to rename it getNextPacket())
- On every return statement in getNextQueueEntry/getNextPacket:
- if it's of the form 'return std::make_tuple(mshr, nullptr)' change it to
'return makePacketFromMSHR(mshr)'
- if it's of the form 'return std::make_tuple(nullptr, wq_entry)' change it
to 'return makePacketFromWriteBuffer(wq_entry)'
- Back in sendDeferredPacket(), replace the old line
MSHR *mshr = dynamic_cast<MSHR*>(pkt->senderState);
with
QueueEntry *q_entry = dynamic_cast<QueueEntry*>(pkt->senderState);
- Define a virtual function on QueueEntry so you can do
bool delete_pkt = q_entry->deletePkt();
- Define another virtual function on QueueEntry so further down you can do
q_entry->markInService(cache, pending_modified_resp);
I expect you may not like the last three items, but I don't see the virtual
functions as making the queue entries "clever", it's really just using virtual
function dispatch to more cleanly implement switching based on a type. I hope
you'll agree that the first four items constitute an unequivocal improvement
though.
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3346/#review8054
-----------------------------------------------------------
On Feb. 24, 2016, 1:28 a.m., Andreas Hansson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3346/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2016, 1:28 a.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> Changeset 11352:3fc299c59057
> ---------------------------
> mem: Create a separate class for the cache write buffer
>
> This patch breaks out the cache write buffer into a separate class,
> without affecting any stats. The goal of the patch is to avoid
> encumbering the much-simpler write queue with the complex MSHR
> handling. In a follow on patch this simplification allows us to
> implement write combining.
>
> The WriteQueue gets its own class, but shares a common ancestor, the
> generic Queue, with the MSHRQueue. To distinguish between
> WriteQueueEntries and MSHRs in the cache, a number of functions are
> returning C++11 tuples as opposed to a single value.
>
>
> Diffs
> -----
>
> src/mem/cache/SConscript ef6e57ac0d70
> src/mem/cache/base.hh ef6e57ac0d70
> src/mem/cache/base.cc ef6e57ac0d70
> src/mem/cache/cache.hh ef6e57ac0d70
> src/mem/cache/cache.cc ef6e57ac0d70
> src/mem/cache/mshr.hh ef6e57ac0d70
> src/mem/cache/mshr.cc ef6e57ac0d70
> src/mem/cache/mshr_queue.hh ef6e57ac0d70
> src/mem/cache/mshr_queue.cc ef6e57ac0d70
> src/mem/cache/queue.hh PRE-CREATION
> src/mem/cache/queue_entry.hh PRE-CREATION
> src/mem/cache/write_queue.hh PRE-CREATION
> src/mem/cache/write_queue.cc PRE-CREATION
> src/mem/cache/write_queue_entry.hh PRE-CREATION
> src/mem/cache/write_queue_entry.cc PRE-CREATION
>
> Diff: http://reviews.gem5.org/r/3346/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andreas Hansson
>
>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
