> On March 13, 2016, 9:39 p.m., Steve Reinhardt wrote:
> > Thanks for all the changes\\\! This is very close to what I was picturing,
> > just one turn of the crank away (assuming you agree with the destination).
> > All it would take would be to:
> >
> > \- Move the calls to checkConflictingSnoop() into
> > sendMSHRQueuePacket()/sendWriteQueuePacket()
> > \- Replace the returns in getNextQueueEntry() with calls to
> > send*QueuePacket()
> >
> > Looking more closely I think I see why you didn't do this, which is that
> > checkConflictingSnoop is a method on CacheReqPacketQueue which is maybe a
> > pointer you don't have once you're in send*QueuePacket(). Can we get back
> > to that structure via memSidePort, or if not, just pass it in as an
> > argument through getNextQueueEntry() to send*QueuePacket()?
>
> Andreas Hansson wrote:
> I do not think what you are proposing is achievable. The
> sendDeferredPacket method is called on the RequestPacketQueue, and it needs
> to update waitingOnRetry, and it also needs to be able to "suspend"
> schedulign if there is a conflict in the snoop response queue. Hence, the
> call to check for conflicts cannot be moved out of this method in any
> sensible fashion without returning tuples/pairs.
>
> Similarly, moving the call to the send function would complicate the call
> chain as the responsibility for the scheduling of events lies with the queue.
>
> The changes you propose may seem simple, but they completely muck with
> the subdivision of responsibilities.
So the issue is that, if we do what I'm proposing, we would not be able to
simply return a bool from the hypothetical sendNextPacket() method because
there are actually three possible outcomes:
if there is a conflict in the snoop queue:
leave waitingOnRetry as false, but don't schedule another event
else if we tried to send but failed:
set waitingOnRetry to true and don't schedule another event
else:
set/leave waitingOnRetry as false and schedule another event to send the
next packet
Right? I figured there was something I was missing...
So given that we can't push these remaining bits of sendDeferredPacket() down
into the part of the code that's already bifurcated according to MSHR vs. write
queue entry, I have to say that I would still find it far more elegant if we
could take this part and rewrite it as something like:
QueueEntry *q_entry = cache.getNextQueueEntry();
if (q_entry) {
if (checkConflictingSnoop(q_entry->blkAddr)) {
return;
}
waitingOnRetry = cache.sendPacket(q_entry);
}
which of course doesn't work because it requires dispatching cache.sendPacket()
on the dynamic type of the q_entry parameter... which sort of looks like
multiple dispatch, but isn't really, since we're still only dispatching on a
single type. So we could convert this to workable C++ by rewriting that one
line as
waitingOnRetry = q_entry->sendPacket(cache);
and then providing appropriate virtual function definitions on MSHR and
WriteQueueEntry to call back to the cache, e.g.,
bool MSHR::sendPacket(Cache &c) { return c.sendMSHRQueuePacket(this); }
bool WriteQueueEntry::sendPacket(Cache &c) { return
c.sendWriteQueuePacket(this); }
I expect you won't like how that makes the control flow less obvious, but to me
the existing solution is a painful repudiation of the entire purpose of type
inheritance. But we've been beating on this for a while now, so I guess if you
really think the tuple thing is preferable, I won't fight it any more.
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3346/#review8082
-----------------------------------------------------------
On March 13, 2016, 5:46 p.m., Andreas Hansson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3346/
> -----------------------------------------------------------
>
> (Updated March 13, 2016, 5:46 p.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> Changeset 11369:ebe8d4868008
> ---------------------------
> 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 2fd64ea0a7cb
> src/mem/cache/base.hh 2fd64ea0a7cb
> src/mem/cache/base.cc 2fd64ea0a7cb
> src/mem/cache/cache.hh 2fd64ea0a7cb
> src/mem/cache/cache.cc 2fd64ea0a7cb
> src/mem/cache/mshr.hh 2fd64ea0a7cb
> src/mem/cache/mshr.cc 2fd64ea0a7cb
> src/mem/cache/mshr_queue.hh 2fd64ea0a7cb
> src/mem/cache/mshr_queue.cc 2fd64ea0a7cb
> 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
