mem: Do not rely on the NeedsWritable flag for responses

This patch removes the NeedsWritable flag for all responses, as it is
really only the request that needs a writable response. The response,
on the other hand, should in these cases always provide the line in a
writable state, as indicated by the hasSharers flag not being set.

When we send requests that has NeedsWritable set, the response will
always have the hasSharers flag not set. Additionally, there are cases
where the request did not have NeedsWritable set, and we still get a
writable response with the hasSharers flag not set. This never happens
on snoops, but is used by downstream caches to pass ownership
upstream.

As part of this patch, the affected response types are updated, and
the snoop filter is similarly modified to check only the hasSharers
flag (as it should). A sanity check is also added to the packet class,
asserting that we never look at the NeedsWritable flag for responses.

No regressions are affected.

3 files changed, 31 insertions, 29 deletions

diff --git a/src/mem/packet.cc b/src/mem/packet.cc
index 289bc81bc..cacfc1d3b 100644
--- a/src/mem/packet.cc
+++ b/src/mem/packet.cc
@@ -83,7 +83,7 @@ MemCmd::commandInfo[] =
     { SET5(IsWrite, NeedsWritable, IsRequest, NeedsResponse, HasData),
             WriteResp, "WriteReq" },
     /* WriteResp */
-    { SET3(IsWrite, NeedsWritable, IsResponse), InvalidCmd, "WriteResp" },
+    { SET2(IsWrite, IsResponse), InvalidCmd, "WriteResp" },
     /* WritebackDirty */
     { SET4(IsWrite, IsRequest, IsEviction, HasData),
             InvalidCmd, "WritebackDirty" },
@@ -117,7 +117,7 @@ MemCmd::commandInfo[] =
            IsRequest, NeedsResponse),
             UpgradeResp, "SCUpgradeReq" },
     /* UpgradeResp */
-    { SET3(NeedsWritable, IsUpgrade, IsResponse),
+    { SET2(IsUpgrade, IsResponse),
             InvalidCmd, "UpgradeResp" },
     /* SCUpgradeFailReq: generates UpgradeFailResp but still gets the data */
     { SET6(IsRead, NeedsWritable, IsInvalidate,
@@ -125,7 +125,7 @@ MemCmd::commandInfo[] =
             UpgradeFailResp, "SCUpgradeFailReq" },
     /* UpgradeFailResp - Behaves like a ReadExReq, but notifies an SC
      * that it has failed, acquires line as Dirty*/
-    { SET4(IsRead, NeedsWritable, IsResponse, HasData),
+    { SET3(IsRead, IsResponse, HasData),
             InvalidCmd, "UpgradeFailResp" },
     /* ReadExReq - Read issues by a cache, always cache-line aligned,
      * and the response is guaranteed to be writeable (exclusive or
@@ -134,7 +134,7 @@ MemCmd::commandInfo[] =
             ReadExResp, "ReadExReq" },
     /* ReadExResp - Response matching a read exclusive, as we check
      * the need for exclusive also on responses */
-    { SET4(IsRead, NeedsWritable, IsResponse, HasData),
+    { SET3(IsRead, IsResponse, HasData),
             InvalidCmd, "ReadExResp" },
     /* ReadCleanReq - Read issued by a cache, always cache-line
      * aligned, and the response is guaranteed to not contain dirty data
@@ -157,13 +157,13 @@ MemCmd::commandInfo[] =
            IsRequest, NeedsResponse, HasData),
             StoreCondResp, "StoreCondFailReq" },
     /* StoreCondResp */
-    { SET4(IsWrite, NeedsWritable, IsLlsc, IsResponse),
+    { SET3(IsWrite, IsLlsc, IsResponse),
             InvalidCmd, "StoreCondResp" },
     /* SwapReq -- for Swap ldstub type operations */
     { SET6(IsRead, IsWrite, NeedsWritable, IsRequest, HasData, NeedsResponse),
         SwapResp, "SwapReq" },
     /* SwapResp -- for Swap ldstub type operations */
-    { SET5(IsRead, IsWrite, NeedsWritable, IsResponse, HasData),
+    { SET4(IsRead, IsWrite, IsResponse, HasData),
             InvalidCmd, "SwapResp" },
     /* IntReq -- for interrupts */
     { SET4(IsWrite, IsRequest, NeedsResponse, HasData),
@@ -190,7 +190,7 @@ MemCmd::commandInfo[] =
     { SET4(IsInvalidate, IsRequest, NeedsWritable, NeedsResponse),
       InvalidateResp, "InvalidateReq" },
     /* Invalidation Response */
-    { SET3(IsInvalidate, IsResponse, NeedsWritable),
+    { SET2(IsInvalidate, IsResponse),
       InvalidCmd, "InvalidateResp" }
 };
 
diff --git a/src/mem/packet.hh b/src/mem/packet.hh
index 94508f697..32c4cf631 100644
--- a/src/mem/packet.hh
+++ b/src/mem/packet.hh
@@ -502,7 +502,15 @@ class Packet : public Printable
     bool isUpgrade()  const          { return cmd.isUpgrade(); }
     bool isRequest() const           { return cmd.isRequest(); }
     bool isResponse() const          { return cmd.isResponse(); }
-    bool needsWritable() const       { return cmd.needsWritable(); }
+    bool needsWritable() const
+    {
+        // we should never check if a response needsWritable, the
+        // request has this flag, and for a response we should rather
+        // look at the hasSharers flag (if not set, the response is to
+        // be considered writable)
+        assert(isRequest());
+        return cmd.needsWritable();
+    }
     bool needsResponse() const       { return cmd.needsResponse(); }
     bool isInvalidate() const        { return cmd.isInvalidate(); }
     bool isEviction() const          { return cmd.isEviction(); }
diff --git a/src/mem/snoop_filter.cc b/src/mem/snoop_filter.cc
index b1ccc12c9..9d02ed249 100755
--- a/src/mem/snoop_filter.cc
+++ b/src/mem/snoop_filter.cc
@@ -257,17 +257,14 @@ SnoopFilter::updateSnoopResponse(const Packet* cpkt,
     panic_if(!(sf_item.requested & req_mask), "SF value %x.%x missing "\
              "the original request\n",  sf_item.requested, sf_item.holder);
 
-    // Update the residency of the cache line.
-    if (cpkt->needsWritable() || !cpkt->hasSharers()) {
-        DPRINTF(SnoopFilter, "%s: dropping %x because needs: %i writable: %i "\
-                "SF val: %x.%x\n", __func__,  rsp_mask,
-                cpkt->needsWritable(), !cpkt->hasSharers(),
+    // If the snoop response has no sharers the line is passed in
+    // Modified state, and we know that there are no other copies, or
+    // they will all be invalidated imminently
+    if (!cpkt->hasSharers()) {
+        DPRINTF(SnoopFilter,
+                "%s: dropping %x because non-shared snoop "
+                "response SF val: %x.%x\n", __func__,  rsp_mask,
                 sf_item.requested, sf_item.holder);
-
-        sf_item.holder &= ~rsp_mask;
-        // The snoop filter does not see any ACKs from non-responding sharers
-        // that have been invalidated :(  So below assert would be nice, but..
-        //assert(sf_item.holder == 0);
         sf_item.holder = 0;
     }
     assert(!cpkt->isWriteback());
@@ -302,14 +299,10 @@ SnoopFilter::updateSnoopForward(const Packet* cpkt,
     DPRINTF(SnoopFilter, "%s:   old SF value %x.%x\n",
             __func__,  sf_item.requested, sf_item.holder);
 
-    // Remote (to this snoop filter) snoops update the filter
-    // already when they arrive from below, because we may not see
-    // any response.
-    if (cpkt->needsWritable()) {
-        // If the request to this snoop response hit an in-flight
-        // transaction,
-        // the holder was not reset -> no assertion & do that here, now!
-        //assert(sf_item.holder == 0);
+    // If the snoop response has no sharers the line is passed in
+    // Modified state, and we know that there are no other copies, or
+    // they will all be invalidated imminently
+    if (!cpkt->hasSharers()) {
         sf_item.holder = 0;
     }
     DPRINTF(SnoopFilter, "%s:   new SF value %x.%x\n",
@@ -343,9 +336,10 @@ SnoopFilter::updateResponse(const Packet* cpkt, const SlavePort& slave_port)
     panic_if(!(sf_item.requested & slave_mask), "SF value %x.%x missing "\
              "request bit\n", sf_item.requested, sf_item.holder);
 
-    // Update the residency of the cache line. Here we assume that the
-    // line has been zapped in all caches that are not the responder.
-     if (cpkt->needsWritable() || !cpkt->hasSharers())
+    // Update the residency of the cache line. If the response has no
+    // sharers we know that the line has been invalidated in all
+    // branches that are not where we are responding to.
+     if (!cpkt->hasSharers())
         sf_item.holder = 0;
     sf_item.holder |=  slave_mask;
     sf_item.requested &= ~slave_mask;