-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3346/#review8054
-----------------------------------------------------------
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.
- Steve Reinhardt
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
