From 7da851d1a834fbe6dd02f87884586129786b14a6 Mon Sep 17 00:00:00 2001 From: Andreas Hansson Date: Thu, 30 May 2013 12:54:11 -0400 Subject: mem: Spring cleaning of MSHR and MSHRQueue This patch does some minor tidying up of the MSHR and MSHRQueue. The clean up started as part of some ad-hoc tracing and debugging, but seems worthwhile enough to go in as a separate patch. The highlights of the changes are reduced scoping (private) members where possible, avoiding redundant new/delete, and constructor initialisation to please static code analyzers. --- src/mem/cache/mshr.cc | 94 +++++++++++++++++++-------------------------- src/mem/cache/mshr.hh | 81 +++++++++++++++++++------------------- src/mem/cache/mshr_queue.cc | 18 +++------ src/mem/cache/mshr_queue.hh | 23 +++++------ 4 files changed, 95 insertions(+), 121 deletions(-) (limited to 'src/mem') diff --git a/src/mem/cache/mshr.cc b/src/mem/cache/mshr.cc index ac16568e8..f96c5c1a7 100644 --- a/src/mem/cache/mshr.cc +++ b/src/mem/cache/mshr.cc @@ -61,13 +61,12 @@ using namespace std; -MSHR::MSHR() +MSHR::MSHR() : readyTime(0), _isUncacheable(false), downstreamPending(false), + pendingDirty(false), postInvalidate(false), + postDowngrade(false), queue(NULL), order(0), addr(0), size(0), + inService(false), isForward(false), threadNum(InvalidThreadID), + data(NULL) { - inService = false; - ntargets = 0; - threadNum = InvalidThreadID; - targets = new TargetList(); - deferredTargets = new TargetList(); } @@ -215,14 +214,13 @@ MSHR::allocate(Addr _addr, int _size, PacketPtr target, inService = false; downstreamPending = false; threadNum = 0; - ntargets = 1; - assert(targets->isReset()); + assert(targets.isReset()); // Don't know of a case where we would allocate a new MSHR for a // snoop (mem-side request), so set source according to request here Target::Source source = (target->cmd == MemCmd::HardPFReq) ? Target::FromPrefetcher : Target::FromCPU; - targets->add(target, whenReady, _order, source, true); - assert(deferredTargets->isReset()); + targets.add(target, whenReady, _order, source, true); + assert(deferredTargets.isReset()); data = NULL; } @@ -234,7 +232,7 @@ MSHR::clearDownstreamPending() downstreamPending = false; // recursively clear flag on any MSHRs we will be forwarding // responses to - targets->clearDownstreamPending(); + targets.clearDownstreamPending(); } bool @@ -249,14 +247,14 @@ MSHR::markInService(PacketPtr pkt) return true; } inService = true; - pendingDirty = (targets->needsExclusive || + pendingDirty = (targets.needsExclusive || (!pkt->sharedAsserted() && pkt->memInhibitAsserted())); postInvalidate = postDowngrade = false; if (!downstreamPending) { // let upstream caches know that the request has made it to a // level where it's going to get a response - targets->clearDownstreamPending(); + targets.clearDownstreamPending(); } return false; } @@ -265,10 +263,9 @@ MSHR::markInService(PacketPtr pkt) void MSHR::deallocate() { - assert(targets->empty()); - targets->resetFlags(); - assert(deferredTargets->isReset()); - assert(ntargets == 0); + assert(targets.empty()); + targets.resetFlags(); + assert(deferredTargets.isReset()); inService = false; } @@ -294,22 +291,20 @@ MSHR::allocateTarget(PacketPtr pkt, Tick whenReady, Counter _order) assert(pkt->cmd != MemCmd::HardPFReq); if (inService && - (!deferredTargets->empty() || hasPostInvalidate() || + (!deferredTargets.empty() || hasPostInvalidate() || (pkt->needsExclusive() && (!isPendingDirty() || hasPostDowngrade() || isForward)))) { // need to put on deferred list if (hasPostInvalidate()) replaceUpgrade(pkt); - deferredTargets->add(pkt, whenReady, _order, Target::FromCPU, true); + deferredTargets.add(pkt, whenReady, _order, Target::FromCPU, true); } else { // No request outstanding, or still OK to append to // outstanding request: append to regular target list. Only // mark pending if current request hasn't been issued yet // (isn't in service). - targets->add(pkt, whenReady, _order, Target::FromCPU, !inService); + targets.add(pkt, whenReady, _order, Target::FromCPU, !inService); } - - ++ntargets; } bool @@ -332,8 +327,8 @@ MSHR::handleSnoop(PacketPtr pkt, Counter _order) // local bus first, some other invalidating transaction // reached the global bus before the upgrade did. if (pkt->needsExclusive()) { - targets->replaceUpgrades(); - deferredTargets->replaceUpgrades(); + targets.replaceUpgrades(); + deferredTargets.replaceUpgrades(); } return false; @@ -344,7 +339,7 @@ MSHR::handleSnoop(PacketPtr pkt, Counter _order) if (pkt->needsExclusive()) { // snooped request still precedes the re-request we'll have to // issue for deferred targets, if any... - deferredTargets->replaceUpgrades(); + deferredTargets.replaceUpgrades(); } if (hasPostInvalidate()) { @@ -365,9 +360,8 @@ MSHR::handleSnoop(PacketPtr pkt, Counter _order) // Actual target device (typ. a memory) will delete the // packet on reception, so we need to save a copy here. PacketPtr cp_pkt = new Packet(pkt, true); - targets->add(cp_pkt, curTick(), _order, Target::FromSnoop, - downstreamPending && targets->needsExclusive); - ++ntargets; + targets.add(cp_pkt, curTick(), _order, Target::FromSnoop, + downstreamPending && targets.needsExclusive); if (isPendingDirty()) { pkt->assertMemInhibit(); @@ -394,23 +388,19 @@ MSHR::handleSnoop(PacketPtr pkt, Counter _order) bool MSHR::promoteDeferredTargets() { - assert(targets->empty()); - if (deferredTargets->empty()) { + assert(targets.empty()); + if (deferredTargets.empty()) { return false; } // swap targets & deferredTargets lists - TargetList *tmp = targets; - targets = deferredTargets; - deferredTargets = tmp; - - assert(targets->size() == ntargets); + std::swap(targets, deferredTargets); // clear deferredTargets flags - deferredTargets->resetFlags(); + deferredTargets.resetFlags(); - order = targets->front().order; - readyTime = std::max(curTick(), targets->front().readyTime); + order = targets.front().order; + readyTime = std::max(curTick(), targets.front().readyTime); return true; } @@ -421,7 +411,7 @@ MSHR::handleFill(Packet *pkt, CacheBlk *blk) { if (!pkt->sharedAsserted() && !(hasPostInvalidate() || hasPostDowngrade()) - && deferredTargets->needsExclusive) { + && deferredTargets.needsExclusive) { // We got an exclusive response, but we have deferred targets // which are waiting to request an exclusive copy (not because // of a pending invalidate). This can happen if the original @@ -430,15 +420,15 @@ MSHR::handleFill(Packet *pkt, CacheBlk *blk) // MOESI/MESI protocol. Since we got the exclusive copy // there's no need to defer the targets, so move them up to // the regular target list. - assert(!targets->needsExclusive); - targets->needsExclusive = true; + assert(!targets.needsExclusive); + targets.needsExclusive = true; // if any of the deferred targets were upper-level cache // requests marked downstreamPending, need to clear that assert(!downstreamPending); // not pending here anymore - deferredTargets->clearDownstreamPending(); + deferredTargets.clearDownstreamPending(); // this clears out deferredTargets too - targets->splice(targets->end(), *deferredTargets); - deferredTargets->resetFlags(); + targets.splice(targets.end(), deferredTargets); + deferredTargets.resetFlags(); } } @@ -453,8 +443,8 @@ MSHR::checkFunctional(PacketPtr pkt) pkt->checkFunctional(this, addr, size, NULL); return false; } else { - return (targets->checkFunctional(pkt) || - deferredTargets->checkFunctional(pkt)); + return (targets.checkFunctional(pkt) || + deferredTargets.checkFunctional(pkt)); } } @@ -474,10 +464,10 @@ MSHR::print(std::ostream &os, int verbosity, const std::string &prefix) const hasPostDowngrade() ? "PostDowngr" : ""); ccprintf(os, "%s Targets:\n", prefix); - targets->print(os, verbosity, prefix + " "); - if (!deferredTargets->empty()) { + targets.print(os, verbosity, prefix + " "); + if (!deferredTargets.empty()) { ccprintf(os, "%s Deferred Targets:\n", prefix); - deferredTargets->print(os, verbosity, prefix + " "); + deferredTargets.print(os, verbosity, prefix + " "); } } @@ -488,9 +478,3 @@ MSHR::print() const print(str); return str.str(); } - -MSHR::~MSHR() -{ - delete[] targets; - delete[] deferredTargets; -} diff --git a/src/mem/cache/mshr.hh b/src/mem/cache/mshr.hh index f99a293fd..c9c30b3e6 100644 --- a/src/mem/cache/mshr.hh +++ b/src/mem/cache/mshr.hh @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012 ARM Limited + * Copyright (c) 2012-2013 ARM Limited * All rights reserved. * * The license below extends only to copyright in the software and shall @@ -64,6 +64,31 @@ class MSHRQueue; class MSHR : public Packet::SenderState, public Printable { + /** + * Consider the MSHRQueue a friend to avoid making everything public + */ + friend class MSHRQueue; + + private: + + /** Cycle when ready to issue */ + Tick readyTime; + + /** True if the request is uncacheable */ + bool _isUncacheable; + + /** Flag set by downstream caches */ + bool downstreamPending; + + /** Will we have a dirty copy after this request? */ + bool pendingDirty; + + /** Did we snoop an invalidate while waiting for data? */ + bool postInvalidate; + + /** Did we snoop a read while waiting for data? */ + bool postDowngrade; + public: class Target { @@ -121,9 +146,6 @@ class MSHR : public Packet::SenderState, public Printable /** Pointer to queue containing this MSHR. */ MSHRQueue *queue; - /** Cycle when ready to issue */ - Tick readyTime; - /** Order number assigned by the miss queue. */ Counter order; @@ -139,42 +161,30 @@ class MSHR : public Packet::SenderState, public Printable /** True if the request is just a simple forward from an upper level */ bool isForward; - /** True if we need to get an exclusive copy of the block. */ - bool needsExclusive() const { return targets->needsExclusive; } - - /** True if the request is uncacheable */ - bool _isUncacheable; - - bool downstreamPending; - /** The pending* and post* flags are only valid if inService is * true. Using the accessor functions lets us detect if these * flags are accessed improperly. */ - /** Will we have a dirty copy after this request? */ - bool pendingDirty; + /** True if we need to get an exclusive copy of the block. */ + bool needsExclusive() const { return targets.needsExclusive; } + bool isPendingDirty() const { assert(inService); return pendingDirty; } - /** Did we snoop an invalidate while waiting for data? */ - bool postInvalidate; bool hasPostInvalidate() const { assert(inService); return postInvalidate; } - /** Did we snoop a read while waiting for data? */ - bool postDowngrade; bool hasPostDowngrade() const { assert(inService); return postDowngrade; } /** Thread number of the miss. */ ThreadID threadNum; - /** The number of currently allocated targets. */ - unsigned short ntargets; + private: /** Data buffer (if needed). Currently used only for pending * upgrade handling. */ @@ -192,15 +202,14 @@ class MSHR : public Packet::SenderState, public Printable */ Iterator allocIter; -private: /** List of all requests that match the address */ - TargetList *targets; + TargetList targets; - TargetList *deferredTargets; + TargetList deferredTargets; -public: + public: - bool isUncacheable() { return _isUncacheable; } + bool isUncacheable() const { return _isUncacheable; } /** * Allocate a miss to this MSHR. @@ -231,35 +240,28 @@ public: /** A simple constructor. */ MSHR(); - /** A simple destructor. */ - ~MSHR(); /** * Returns the current number of allocated targets. * @return The current number of allocated targets. */ - int getNumTargets() const { return ntargets; } - - /** - * Returns a pointer to the target list. - * @return a pointer to the target list. - */ - TargetList *getTargetList() { return targets; } + int getNumTargets() const + { return targets.size() + deferredTargets.size(); } /** * Returns true if there are targets left. * @return true if there are targets */ - bool hasTargets() const { return !targets->empty(); } + bool hasTargets() const { return !targets.empty(); } /** * Returns a reference to the first target. * @return A pointer to the first target. */ - Target *getTarget() const + Target *getTarget() { assert(hasTargets()); - return &targets->front(); + return &targets.front(); } /** @@ -267,15 +269,14 @@ public: */ void popTarget() { - --ntargets; - targets->pop_front(); + targets.pop_front(); } bool isForwardNoResponse() const { if (getNumTargets() != 1) return false; - Target *tgt = getTarget(); + const Target *tgt = &targets.front(); return tgt->source == Target::FromCPU && !tgt->pkt->needsResponse(); } diff --git a/src/mem/cache/mshr_queue.cc b/src/mem/cache/mshr_queue.cc index af13d12d3..d8cc5f40a 100644 --- a/src/mem/cache/mshr_queue.cc +++ b/src/mem/cache/mshr_queue.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012 ARM Limited + * Copyright (c) 2012-2013 ARM Limited * All rights reserved. * * The license below extends only to copyright in the software and shall @@ -51,24 +51,16 @@ using namespace std; MSHRQueue::MSHRQueue(const std::string &_label, int num_entries, int reserve, int _index) - : label(_label), - numEntries(num_entries + reserve - 1), numReserve(reserve), - drainManager(NULL), index(_index) + : label(_label), numEntries(num_entries + reserve - 1), + numReserve(reserve), registers(numEntries), + drainManager(NULL), allocated(0), inServiceEntries(0), index(_index) { - allocated = 0; - inServiceEntries = 0; - registers = new MSHR[numEntries]; for (int i = 0; i < numEntries; ++i) { registers[i].queue = this; freeList.push_back(®isters[i]); } } -MSHRQueue::~MSHRQueue() -{ - delete [] registers; -} - MSHR * MSHRQueue::findMatch(Addr addr) const { @@ -253,7 +245,7 @@ MSHRQueue::squash(int threadNum) assert(0/*target->req->threadId()*/ == threadNum); } assert(!mshr->hasTargets()); - assert(mshr->ntargets==0); + assert(mshr->getNumTargets()==0); if (!mshr->inService) { i = deallocateOne(mshr); } else { diff --git a/src/mem/cache/mshr_queue.hh b/src/mem/cache/mshr_queue.hh index 44e1c5bd3..726aa6b8e 100644 --- a/src/mem/cache/mshr_queue.hh +++ b/src/mem/cache/mshr_queue.hh @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012 ARM Limited + * Copyright (c) 2012-2013 ARM Limited * All rights reserved. * * The license below extends only to copyright in the software and shall @@ -63,15 +63,6 @@ class MSHRQueue : public Drainable /** Local label (for functional print requests) */ const std::string label; - /** MSHR storage. */ - MSHR *registers; - /** Holds pointers to all allocated entries. */ - MSHR::List allocatedList; - /** Holds pointers to entries that haven't been sent to the bus. */ - MSHR::List readyList; - /** Holds non allocated entries. */ - MSHR::List freeList; - // Parameters /** * The total number of entries in this queue. This number is set as the @@ -86,6 +77,15 @@ class MSHRQueue : public Drainable */ const int numReserve; + /** MSHR storage. */ + std::vector registers; + /** Holds pointers to all allocated entries. */ + MSHR::List allocatedList; + /** Holds pointers to entries that haven't been sent to the bus. */ + MSHR::List readyList; + /** Holds non allocated entries. */ + MSHR::List freeList; + /** Drain manager to inform of a completed drain */ DrainManager *drainManager; @@ -110,9 +110,6 @@ class MSHRQueue : public Drainable MSHRQueue(const std::string &_label, int num_entries, int reserve, int index); - /** Destructor */ - ~MSHRQueue(); - /** * Find the first MSHR that matches the provided address. * @param addr The address to find. -- cgit v1.2.3