> On March 4, 2016, 5:13 p.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?
>
> Steve Reinhardt wrote:
> 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.
>
> Andreas Hansson wrote:
> I can play around with the functions and how they interlink. That said,
> I'd rather deal with the tuple than the dynamic_cast. Also, the MSHR and
> WriteQueueEntry already have a markInService that is called by the
> markInService on the MSHRQueue and WriteQueue, so things would get very
> convoluted if we add another function on the entry, calling the queue,
> calling the entry. I'll have a think around this, but I really don't want to
> confuse things unecessarily.
>
> Steve Reinhardt wrote:
> I'd be fine with having getNextQueueEntry() return a tuple<PacketPtr,
> QueueEntry*>... the clarity and the avoidance of the dynamic_cast outweighs
> the redundancy of explicitly returning something that's already available via
> pkt->senderState().
>
> Basically the entry and the queue both have markInService() code that
> needs to be executed, but I'm not sure that it matters which one calls which.
> Currently the queue function calls the entry function, but it could go the
> other way around.
So this is proving more challenging than what I hoped. markInService ultimately
needs to be called on the cache, and it has different signature and behaviour
for MSHRs and write queue entries. Even if the entry could call the queue, the
queue has no sensible way of calling the cache. Also the flow from entry to
queue to cache seems very contrived to me.
I am afraid I do not really see a sensible way of avoiding the current
structure.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3346/#review8054
-----------------------------------------------------------
On Feb. 24, 2016, 9: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, 9: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
