From fec788d648d20fbd983e69066c6c82cdf062bb12 Mon Sep 17 00:00:00 2001
From: Hansung Kim <hansung_kim@berkeley.edu>
Date: Fri, 28 Apr 2023 15:46:45 -0700
Subject: [PATCH] Invalidate head when dequeued but allowShift was false

---
 src/main/scala/tilelink/Coalescing.scala      |  5 +++
 .../scala/coalescing/CoalescingUnitTest.scala | 45 +++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/src/main/scala/tilelink/Coalescing.scala b/src/main/scala/tilelink/Coalescing.scala
index 2590884..56c99aa 100644
--- a/src/main/scala/tilelink/Coalescing.scala
+++ b/src/main/scala/tilelink/Coalescing.scala
@@ -213,6 +213,11 @@ class CoalShiftQueue[T <: Data]( gen: T,
       (io.queue.enq.fire && !paddedUsed(i + 1) && used(i)) || pad(validAfterInv)(i + 1),
       (io.queue.enq.fire && paddedUsed(i - 1) && !used(i)) || validAfterInv(i)
     )
+    // additionally, head entry should get invalidated when dequeue fired
+    // but queue didn't shift (e.g. because allowShift was false)
+    when (io.queue.deq.fire && !shift) {
+      valid(0) := false.B
+    }
   }
 
   when(io.queue.enq.fire) {
diff --git a/src/test/scala/coalescing/CoalescingUnitTest.scala b/src/test/scala/coalescing/CoalescingUnitTest.scala
index 20b95c1..49a5639 100644
--- a/src/test/scala/coalescing/CoalescingUnitTest.scala
+++ b/src/test/scala/coalescing/CoalescingUnitTest.scala
@@ -302,6 +302,43 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
     }
   }
 
+  it should "not shift entries when allowShift is false" in {
+    test(new CoalShiftQueue(UInt(8.W), 4)) { c =>
+      c.io.invalidate.valid.poke(false.B)
+      c.io.queue.deq.ready.poke(false.B)
+
+      c.io.allowShift.poke(false.B)
+
+      // prepare
+      c.io.queue.enq.ready.expect(true.B)
+      c.io.queue.enq.valid.poke(true.B)
+      c.io.queue.enq.bits.poke(0x12.U)
+      c.clock.step()
+      c.io.queue.enq.ready.expect(true.B)
+      c.io.queue.enq.valid.poke(true.B)
+      c.io.queue.enq.bits.poke(0x34.U)
+      c.clock.step()
+      c.io.queue.enq.valid.poke(false.B)
+
+      // dequeueing should work normally when allowShift is false...
+      c.io.queue.deq.ready.poke(true.B)
+      c.io.queue.deq.valid.expect(true.B)
+      c.io.queue.deq.bits.expect(0x12.U)
+      c.clock.step()
+      // but should stop there and not dequeue the next entry
+      c.io.queue.deq.ready.poke(true.B)
+      c.io.queue.deq.valid.expect(false.B)
+      c.clock.step()
+      // when allowShift is back one, dequeueing should start working from next
+      // cycle
+      c.io.allowShift.poke(true.B)
+      c.clock.step()
+      c.io.queue.deq.ready.poke(true.B)
+      c.io.queue.deq.valid.expect(true.B)
+      c.io.queue.deq.bits.expect(0x34.U)
+    }
+  }
+
   it should "work when enqueing and dequeueing simultaneously to a depth=1 queue" in {
     test(new CoalShiftQueue(UInt(8.W), 1)) { c =>
       c.io.invalidate.valid.poke(false.B)
@@ -358,9 +395,9 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
       c.io.allowShift.poke(false.B)
       c.io.invalidate.valid.poke(true.B)
       c.io.invalidate.bits.poke(0x1.U)
-      // TODO: we might be able to enqueue to a depth=1 queue whose entry
-      // just got invalidated, so that enq.ready is true.B here, but it is a
-      // niche case
+      // TODO: we might be able to enqueue to a full depth=1 queue whose only
+      // entry just got invalidated, so that enq.ready is true here, but
+      // it is a niche case
       c.io.queue.enq.ready.expect(false.B)
       c.clock.step()
       // now try enqueueing now that we have space
@@ -379,7 +416,7 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
     }
   }
 
-  it should "invalidate head being dequeued" in {
+  it should "invalidate head that is also being dequeued" in {
     test(new CoalShiftQueue(UInt(8.W), 4)) { c =>
       c.io.invalidate.valid.poke(false.B)
       c.io.allowShift.poke(true.B)