mem: Encapsulate retry variables of SnoopFilter

Group all variables related to the restoration of a snoop filter
entry due to a crossbar retry.

Change-Id: I4e03edb3afd06563b7a5812959739876709eceeb
Signed-off-by: Daniel R. Carvalho <odanrc@yahoo.com.br>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/19749
Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Maintainer: Nikos Nikoleris <nikos.nikoleris@arm.com>
Tested-by: kokoro <noreply+kokoro@google.com>

2 files changed, 44 insertions, 21 deletions

diff --git a/src/mem/snoop_filter.cc b/src/mem/snoop_filter.cc
index 8168c3d91..d368dd868 100644
--- a/src/mem/snoop_filter.cc
+++ b/src/mem/snoop_filter.cc
@@ -76,8 +76,8 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port)
         line_addr |= LineSecure;
     }
     SnoopMask req_port = portToMask(slave_port);
-    reqLookupResult = cachedLocations.find(line_addr);
-    bool is_hit = (reqLookupResult != cachedLocations.end());
+    reqLookupResult.it = cachedLocations.find(line_addr);
+    bool is_hit = (reqLookupResult.it != 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
@@ -86,15 +86,17 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port)
         return snoopDown(lookupLatency);
 
     // 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;
+    if (!is_hit) {
+        reqLookupResult.it =
+            cachedLocations.emplace(line_addr, SnoopItem()).first;
+    }
+    SnoopItem& sf_item = reqLookupResult.it->second;
     SnoopMask interested = sf_item.holder | sf_item.requested;
 
     // Store unmodified value of snoop filter item in temp storage in
     // case we need to revert because of a send retry in
     // updateRequest.
-    retryItem = sf_item;
+    reqLookupResult.retryItem = sf_item;
 
     totRequests++;
     if (is_hit) {
@@ -155,25 +157,26 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port)
 void
 SnoopFilter::finishRequest(bool will_retry, Addr addr, bool is_secure)
 {
-    if (reqLookupResult != cachedLocations.end()) {
+    if (reqLookupResult.it != cachedLocations.end()) {
         // since we rely on the caller, do a basic check to ensure
         // that finishRequest is being called following lookupRequest
         Addr line_addr = (addr & ~(Addr(linesize - 1)));
         if (is_secure) {
             line_addr |= LineSecure;
         }
-        assert(reqLookupResult->first == line_addr);
+        assert(reqLookupResult.it->first == line_addr);
         if (will_retry) {
+            SnoopItem retry_item = reqLookupResult.retryItem;
             // 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;
+            reqLookupResult.it->second = retry_item;
 
             DPRINTF(SnoopFilter, "%s:   restored SF value %x.%x\n",
-                    __func__,  retryItem.requested, retryItem.holder);
+                    __func__,  retry_item.requested, retry_item.holder);
         }
 
-        eraseIfNullEntry(reqLookupResult);
+        eraseIfNullEntry(reqLookupResult.it);
     }
 }
 
diff --git a/src/mem/snoop_filter.hh b/src/mem/snoop_filter.hh
index 9c066155c..4d1eea944 100644
--- a/src/mem/snoop_filter.hh
+++ b/src/mem/snoop_filter.hh
@@ -94,7 +94,7 @@ class SnoopFilter : public SimObject {
     typedef std::vector<QueuedSlavePort*> SnoopList;
 
     SnoopFilter (const SnoopFilterParams *p) :
-        SimObject(p), reqLookupResult(cachedLocations.end()), retryItem{0, 0},
+        SimObject(p), reqLookupResult(cachedLocations.end()),
         linesize(p->system->cacheLineSize()), lookupLatency(p->lookup_latency),
         maxEntryCount(p->max_capacity / p->system->cacheLineSize())
     {
@@ -261,17 +261,37 @@ 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 finishRequest needs to undo changes made in lookupRequest
-     * (because of crossbar retry)
+     * A request lookup must be followed by a call to finishRequest to inform
+     * the operation's success. If a retry is needed, however, all changes
+     * made to the snoop filter while performing the lookup must be undone.
+     * This structure keeps track of the state previous to such changes.
      */
-    SnoopItem retryItem;
+    struct ReqLookupResult {
+        /** Iterator used to store the result from lookupRequest. */
+        SnoopFilterCache::iterator it;
+
+        /**
+         * Variable to temporarily store value of snoopfilter entry
+         * in case finishRequest needs to undo changes made in lookupRequest
+         * (because of crossbar retry)
+         */
+        SnoopItem retryItem;
+
+        /**
+         * The constructor must be informed of the internal cache's end
+         * iterator, so do not allow the compiler to implictly define it.
+         *
+         * @param end_it Iterator to the end of the internal cache.
+         */
+        ReqLookupResult(SnoopFilterCache::iterator end_it)
+            : it(end_it), retryItem{0, 0}
+        {
+        }
+        ReqLookupResult() = delete;
+    } reqLookupResult;
+
     /** List of all attached snooping slave ports. */
     SnoopList slavePorts;
     /** Track the mapping from port ids to the local mask ids. */