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

@@ -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);
+    targets.add(target, whenReady, _order, source, true);
-    assert(deferredTargets->isReset());
+    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());
+    assert(targets.empty());
-    targets->resetFlags();
+    targets.resetFlags();
-    assert(deferredTargets->isReset());
+    assert(deferredTargets.isReset());
-    assert(ntargets == 0);
     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();
+            targets.replaceUpgrades();
-            deferredTargets->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,
+        targets.add(cp_pkt, curTick(), _order, Target::FromSnoop,
-                     downstreamPending && targets->needsExclusive);
+                     downstreamPending && targets.needsExclusive);
-        ++ntargets;
 
         if (isPendingDirty()) {
             pkt->assertMemInhibit();
@@ -394,23 +388,19 @@ MSHR::handleSnoop(PacketPtr pkt, Counter _order)
 bool
 MSHR::promoteDeferredTargets()
 {
-    assert(targets->empty());
+    assert(targets.empty());
-    if (deferredTargets->empty()) {
+    if (deferredTargets.empty()) {
         return false;
     }
 
     // swap targets & deferredTargets lists
-    TargetList *tmp = targets;
+    std::swap(targets, deferredTargets);
-    targets = deferredTargets;
-    deferredTargets = tmp;
-
-    assert(targets->size() == ntargets);
 
     // clear deferredTargets flags
-    deferredTargets->resetFlags();
+    deferredTargets.resetFlags();
 
-    order = targets->front().order;
+    order = targets.front().order;
-    readyTime = std::max(curTick(), targets->front().readyTime);
+    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);
+        assert(!targets.needsExclusive);
-        targets->needsExclusive = true;
+        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);
+        targets.splice(targets.end(), deferredTargets);
-        deferredTargets->resetFlags();
+        deferredTargets.resetFlags();
     }
 }
 
@@ -453,8 +443,8 @@ MSHR::checkFunctional(PacketPtr pkt)
         pkt->checkFunctional(this, addr, size, NULL);
         return false;
     } else {
-        return (targets->checkFunctional(pkt) ||
+        return (targets.checkFunctional(pkt) ||
-                deferredTargets->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 + "    ");
+    targets.print(os, verbosity, prefix + "    ");
-    if (!deferredTargets->empty()) {
+    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;
-}

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? */
+    /** True if we need to get an exclusive copy of the block. */
-    bool pendingDirty;
+    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; }
+    int getNumTargets() const
-
+    { return targets.size() + deferredTargets.size(); }
-    /**
-     * Returns a pointer to the target list.
-     * @return a pointer to the target list.
-     */
-    TargetList *getTargetList() { return targets; }
 
     /**
      * 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();
     }
 

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),
+    : label(_label), numEntries(num_entries + reserve - 1),
-      numEntries(num_entries + reserve - 1), numReserve(reserve),
+      numReserve(reserve), registers(numEntries),
-      drainManager(NULL), index(_index)
+      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(&registers[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 {

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<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;
+
     /** 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.