From de6d6eee1a923b5e9eb1d02d6c16616c0c17d74d Mon Sep 17 00:00:00 2001
From: Hansung Kim <hansung_kim@berkeley.edu>
Date: Thu, 20 Apr 2023 21:12:19 -0700
Subject: [PATCH] Fix request shift queue not enqueuing when empty

The queue was enabling shifting of the registers whenever deq.ready
was 1, even when the queue was empty.  This caused `wen` to disable
writing enq.bits to any of the entries in the queue.  Fixed by setting
`shift` to 0 when queue is empty.
---
 src/main/scala/tilelink/Coalescing.scala      | 13 +++---
 .../scala/coalescing/CoalescingUnitTest.scala | 41 ++++++++++++++++++-
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/src/main/scala/tilelink/Coalescing.scala b/src/main/scala/tilelink/Coalescing.scala
index ecb8668..7f01413 100644
--- a/src/main/scala/tilelink/Coalescing.scala
+++ b/src/main/scala/tilelink/Coalescing.scala
@@ -125,14 +125,13 @@ class CoalescingUnitImp(outer: CoalescingUnit, numLanes: Int) extends LazyModule
       req.address := tlIn.a.bits.address
       req.data := tlIn.a.bits.data
 
+      assert(reqQueue.io.enq.ready, "reqQueue is supposed to be always ready")
       reqQueue.io.enq.valid := tlIn.a.valid
       reqQueue.io.enq.bits := req
       // TODO: deq.ready should respect downstream ready
       reqQueue.io.deq.ready := true.B
       reqQueue.io.invalidate := 0.U
 
-      printf(s"reqQueue(${lane}).count=%d\n", reqQueue.io.count)
-
       // Invalidate coalesced requests
       // FIXME: hardcoded lanes
       // val invalidate = coalReqValid && (lane == 0 || lane == 2).B
@@ -208,8 +207,9 @@ class CoalescingUnitImp(outer: CoalescingUnit, numLanes: Int) extends LazyModule
   coalReqAddress := (0xabcd.U + coalSourceId) << 4
   // FIXME: bogus coalescing logic: coalesce whenever all 4 lanes have valid
   // queue head
-  coalReqValid := reqQueues(0).io.deq.valid && reqQueues(1).io.deq.valid &&
-    reqQueues(2).io.deq.valid && reqQueues(3).io.deq.valid
+  // coalReqValid := reqQueues(0).io.deq.valid && reqQueues(1).io.deq.valid &&
+  //   reqQueues(2).io.deq.valid && reqQueues(3).io.deq.valid
+  coalReqValid := false.B
   when(coalReqValid) {
     // invalidate original requests due to coalescing
     // FIXME: bogus
@@ -245,7 +245,9 @@ class CoalescingUnitImp(outer: CoalescingUnit, numLanes: Int) extends LazyModule
   val newEntry = Wire(
     new InflightCoalReqTableEntry(numLanes, numPerLaneReqs, sourceWidth, offsetBits, sizeBits)
   )
+
   println(s"=========== table sourceWidth: ${sourceWidth}")
+
   newEntry.source := coalSourceId
   newEntry.lanes.foreach { l =>
     l.reqs.zipWithIndex.foreach { case (r, i) =>
@@ -535,11 +537,12 @@ class CoalShiftQueue[T <: Data](
   def paddedUsed = pad({ i: Int => used(i) })
   def validAfterInv(i: Int) = valid(i) && !io.invalidate(i)
 
-  val shift = io.deq.ready || (used =/= 0.U) && !validAfterInv(0)
+  val shift = (used =/= 0.U) && (io.deq.ready || !validAfterInv(0))
   for (i <- 0 until entries) {
     val wdata = if (i == entries - 1) io.enq.bits else Mux(!used(i + 1), io.enq.bits, elts(i + 1))
     val wen = Mux(
       shift,
+      // enqueue to the top entry which will be shifted down and make space
       (io.enq.fire && !paddedUsed(i + 1) && used(i)) || pad(validAfterInv)(i + 1),
       // enqueue to the first empty slot above the top
       (io.enq.fire && paddedUsed(i - 1) && !used(i)) || !validAfterInv(i)
diff --git a/src/test/scala/coalescing/CoalescingUnitTest.scala b/src/test/scala/coalescing/CoalescingUnitTest.scala
index 47db1b6..2556c35 100644
--- a/src/test/scala/coalescing/CoalescingUnitTest.scala
+++ b/src/test/scala/coalescing/CoalescingUnitTest.scala
@@ -87,7 +87,7 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
       c.io.invalidate.poke(0.U)
 
       // prepare
-      c.io.deq.ready.poke(false.B)
+      c.io.deq.ready.poke(true.B)
       c.io.enq.ready.expect(true.B)
       c.io.enq.valid.poke(true.B)
       c.io.enq.bits.poke(0x12.U)
@@ -113,6 +113,45 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
     }
   }
 
+  it should "work when enqueing and dequeueing simultaneously to a full queue" in {
+    test(new CoalShiftQueue(UInt(8.W), 1)) { c =>
+      c.io.invalidate.poke(0.U)
+
+      // prepare
+      c.io.deq.ready.poke(true.B)
+      c.io.enq.ready.expect(true.B)
+      c.io.enq.valid.poke(true.B)
+      c.io.enq.bits.poke(0x12.U)
+      c.clock.step()
+      // enqueue and dequeue simultaneously
+      c.io.deq.ready.poke(true.B)
+      c.io.enq.ready.expect(true.B)
+      c.io.enq.valid.poke(true.B)
+      c.io.enq.bits.poke(0x34.U)
+      c.io.deq.valid.expect(true.B)
+      c.io.deq.bits.expect(0x12.U)
+      c.clock.step()
+      // enqueue and dequeue simultaneously once more
+      c.io.deq.ready.poke(true.B)
+      c.io.enq.ready.expect(true.B)
+      c.io.enq.valid.poke(true.B)
+      c.io.enq.bits.poke(0x56.U)
+      c.io.deq.valid.expect(true.B)
+      c.io.deq.bits.expect(0x34.U)
+      c.clock.step()
+      // dequeueing back-to-back should work without any holes in the middle
+      c.io.deq.ready.poke(true.B)
+      c.io.enq.valid.poke(false.B)
+      c.io.deq.valid.expect(true.B)
+      c.io.deq.bits.expect(0x56.U)
+      c.clock.step()
+      // make sure is empty
+      c.io.deq.ready.poke(true.B)
+      c.io.enq.valid.poke(false.B)
+      c.io.deq.valid.expect(false.B)
+    }
+  }
+
   it should "invalidate head being dequeued" in {
     test(new CoalShiftQueue(UInt(8.W), 4)) { c =>
       c.io.invalidate.poke(0.U)