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?

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.

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.

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.

Btw, I am also not too happy with the tuples, but I don't want to replace one "ugly" construct with another one, especially as I find that the current ugliness at least is easy to understand and follow.

Sorry for the delay again... I was traveling and then caught a cold on the trip.  Don't mean to string you along like this.
So one thing I wonder is whether we're now just trying to hard to force the new reality into the historical framework... yea, there are a lot of parallels in what happens whether you're sending out a miss from the MSHRs or a write from the write buffer, but when you come down to it, we're forcing the two different cases down the same code path just so they can share:

1. The snoop response queue check

2. The call to masterPort.sendTimingReq()

3. The event resched if the send is successful and there are more packets to go
Maybe instead of trying to reconverge the execution before sending the packet, we should take the initial steps I outlined above, only instead of calling makePacketFromMSHR()/makePacketFromWriteBuffer(), we should replace those functions with sendPacketFromMSHR()/sendPacketFromWriteBuffer(), which incorporate some or all of the code after the call to getTimingPacket() in sendDeferredPacket() as well.  (And we'd probably want to rename getTimingPacket() to sendTimingPacket().)  We could put the snoop response queue check (item 1 above) in a function that gets called from both places to reduce redundancy.  Item 2 is just one line so we could live with replicating it.  Item 3 might be something we could leave in sendDeferredPacket(), assuming we return the success/waitingOnRetry flag from the new sendTimingPacket().
Does that make sense?  There's some high-level loss of elegance in that conceptually similar things are now being done in different functions, but there's no need to force-fit different things into the same code path at all.

Have a look. I'm keen to move on to the follow-on patches, so the sooner we can converge on this the better.

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.