mem: Store snoop filter lookup result to avoid second lookup

This patch introduces a private member storing the iterator from the
lookupRequest call, such that it can be re-used when the request
eventually finishes. The method previously called updateRequest is
renamed finishRequest to make it more clear that the two functions
must be called together.

3 files changed, 43 insertions, 41 deletions

diff --git a/src/mem/coherent_xbar.cc b/src/mem/coherent_xbar.cc
index 7181cb965..206f94406 100644
--- a/src/mem/coherent_xbar.cc
+++ b/src/mem/coherent_xbar.cc
@@ -235,7 +235,7 @@ CoherentXBar::recvTimingReq(PacketPtr pkt, PortID slave_port_id)
 
     if (snoopFilter && !system->bypassCaches()) {
         // Let the snoop filter know about the success of the send operation
-        snoopFilter->updateRequest(pkt, *src_port, !success);
+        snoopFilter->finishRequest(!success, pkt);
     }
 
     // check if we were successful in sending the packet onwards
@@ -610,7 +610,7 @@ CoherentXBar::recvAtomic(PacketPtr pkt, PortID slave_port_id)
             // operation, and do it even before sending it onwards to
             // avoid situations where atomic upward snoops sneak in
             // between and change the filter state
-            snoopFilter->updateRequest(pkt, *slavePorts[slave_port_id], false);
+            snoopFilter->finishRequest(false, pkt);
 
             snoop_result = forwardAtomic(pkt, slave_port_id, InvalidPortID,
                                          sf_res.first);
diff --git a/src/mem/snoop_filter.cc b/src/mem/snoop_filter.cc
index a52d86380..f6e6ef1b4 100755
--- a/src/mem/snoop_filter.cc
+++ b/src/mem/snoop_filter.cc
@@ -70,8 +70,8 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port)
     bool allocate = !cpkt->req->isUncacheable() && slave_port.isSnooping();
     Addr line_addr = cpkt->getBlockAddr(linesize);
     SnoopMask req_port = portToMask(slave_port);
-    auto sf_it = cachedLocations.find(line_addr);
-    bool is_hit = (sf_it != cachedLocations.end());
+    reqLookupResult = cachedLocations.find(line_addr);
+    bool is_hit = (reqLookupResult != cachedLocations.end());
 
     // If the snoop filter has no entry, and we should not allocate,
     // do not create a new snoop filter entry, simply return a NULL
@@ -79,8 +79,10 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port)
     if (!is_hit && !allocate)
         return snoopDown(lookupLatency);
 
-    // Create a new element through operator[] and modify in-place
-    SnoopItem& sf_item = is_hit ? sf_it->second : cachedLocations[line_addr];
+    // If no hit in snoop filter create a new element and update iterator
+    if (!is_hit)
+        reqLookupResult = cachedLocations.emplace(line_addr, SnoopItem()).first;
+    SnoopItem& sf_item = reqLookupResult->second;
     SnoopMask interested = sf_item.holder | sf_item.requested;
 
     // Store unmodified value of snoop filter item in temp storage in
@@ -144,32 +146,24 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port)
 }
 
 void
-SnoopFilter::updateRequest(const Packet* cpkt, const SlavePort& slave_port,
-                           bool will_retry)
+SnoopFilter::finishRequest(bool will_retry, const Packet* cpkt)
 {
-    DPRINTF(SnoopFilter, "%s: packet src %s addr 0x%x cmd %s\n",
-            __func__, slave_port.name(), cpkt->getAddr(), cpkt->cmdString());
-
-    // Ultimately we should check if the packet came from an
-    // allocating source, not just if the port is snooping
-    bool allocate = !cpkt->req->isUncacheable() && slave_port.isSnooping();
-    if (!allocate)
-        return;
+    if (reqLookupResult != cachedLocations.end()) {
+        // since we rely on the caller, do a basic check to ensure
+        // that finishRequest is being called following lookupRequest
+        assert(reqLookupResult->first == cpkt->getBlockAddr(linesize));
+        if (will_retry) {
+            // Undo any changes made in lookupRequest to the snoop filter
+            // entry if the request will come again. retryItem holds
+            // the previous value of the snoopfilter entry.
+            reqLookupResult->second = retryItem;
+
+            DPRINTF(SnoopFilter, "%s:   restored SF value %x.%x\n",
+                    __func__,  retryItem.requested, retryItem.holder);
+        }
 
-    Addr line_addr = cpkt->getBlockAddr(linesize);
-    auto sf_it = cachedLocations.find(line_addr);
-    assert(sf_it != cachedLocations.end());
-    if (will_retry) {
-        // Undo any changes made in lookupRequest to the snoop filter
-        // entry if the request will come again. retryItem holds
-        // the previous value of the snoopfilter entry.
-        sf_it->second = retryItem;
-
-        DPRINTF(SnoopFilter, "%s:   restored SF value %x.%x\n",
-                __func__,  retryItem.requested, retryItem.holder);
+        eraseIfNullEntry(reqLookupResult);
     }
-
-    eraseIfNullEntry(sf_it);
 }
 
 std::pair<SnoopFilter::SnoopList, Cycles>
diff --git a/src/mem/snoop_filter.hh b/src/mem/snoop_filter.hh
index ee2c82b6e..b1e33dc8f 100755
--- a/src/mem/snoop_filter.hh
+++ b/src/mem/snoop_filter.hh
@@ -88,7 +88,8 @@ class SnoopFilter : public SimObject {
   public:
     typedef std::vector<QueuedSlavePort*> SnoopList;
 
-    SnoopFilter (const SnoopFilterParams *p) : SimObject(p),
+    SnoopFilter (const SnoopFilterParams *p) :
+        SimObject(p), reqLookupResult(cachedLocations.end()), retryItem{0, 0},
         linesize(p->system->cacheLineSize()), lookupLatency(p->lookup_latency)
     {
     }
@@ -104,10 +105,12 @@ class SnoopFilter : public SimObject {
     }
 
     /**
-     * Lookup a request (from a slave port) in the snoop filter and return a
-     * list of other slave ports that need forwarding of the resulting snoops.
-     * Additionally, update the tracking structures with new request
-     * information.
+     * Lookup a request (from a slave port) in the snoop filter and
+     * return a list of other slave ports that need forwarding of the
+     * resulting snoops.  Additionally, update the tracking structures
+     * with new request information. Note that the caller must also
+     * call finishRequest once it is known if the request needs to
+     * retry or not.
      *
      * @param cpkt          Pointer to the request packet.  Not changed.
      * @param slave_port    Slave port where the request came from.
@@ -117,15 +120,15 @@ class SnoopFilter : public SimObject {
                                                const SlavePort& slave_port);
 
     /**
-     * For a successful request, update all data structures in the snoop filter
-     * reflecting the changes caused by that request
+     * For an un-successful request, revert the change to the snoop
+     * filter. Also take care of erasing any null entries. This method
+     * relies on the result from lookupRequest being stored in
+     * reqLookupResult.
      *
-     * @param cpkt          Pointer to the request packet.  Not changed.
-     * @param slave_port    Slave port where the request came from.
      * @param will_retry    This request will retry on this bus / snoop filter
+     * @param cpkt     Request packet, merely for sanity checking
      */
-    void updateRequest(const Packet* cpkt, const SlavePort& slave_port,
-                       bool will_retry);
+    void finishRequest(bool will_retry, const Packet* cpkt);
 
     /**
      * Handle an incoming snoop from below (the master port).  These can upgrade the
@@ -235,8 +238,13 @@ class SnoopFilter : public SimObject {
     /** Simple hash set of cached addresses. */
     SnoopFilterCache cachedLocations;
     /**
+     * Iterator used to store the result from lookupRequest until we
+     * call finishRequest.
+     */
+    SnoopFilterCache::iterator reqLookupResult;
+    /**
      * Variable to temporarily store value of snoopfilter entry
-     * incase updateRequest needs to undo changes made in lookupRequest
+     * incase finishRequest needs to undo changes made in lookupRequest
      * (because of crossbar retry)
      */
     SnoopItem retryItem;