> 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.
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
-----------------------------------------------------------
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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev
