From 4aabbecda16cef51cdf5efb538f99a2d9926e393 Mon Sep 17 00:00:00 2001
From: Hansung Kim <hansung_kim@berkeley.edu>
Date: Fri, 31 Mar 2023 20:38:21 -0700
Subject: [PATCH] Do not deassert deq.valid right after invalidate(head)

... to avoid combinational cycle.
---
 src/main/scala/tilelink/Coalescing.scala | 23 ++++++++---------------
 src/test/scala/CoalescingUnitTest.scala  | 11 ++++++-----
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/src/main/scala/tilelink/Coalescing.scala b/src/main/scala/tilelink/Coalescing.scala
index 80c7a63..9c2d111 100644
--- a/src/main/scala/tilelink/Coalescing.scala
+++ b/src/main/scala/tilelink/Coalescing.scala
@@ -460,7 +460,8 @@ class InflightCoalReqTableEntry(
 // Mostly copied from freechips.rocketchip.util.ShiftQueue, except that every
 // queue entry and its valid signal are exposed as output IO.
 // If `pipe` is true, support enqueueing to a full queue when also dequeueing.
-// TODO: support invalidate and deadline
+//
+// TODO: support deadline
 class CoalShiftQueue[T <: Data](
     gen: T,
     val entries: Int,
@@ -522,7 +523,12 @@ class CoalShiftQueue[T <: Data](
   }
 
   io.enq.ready := !valid(entries - 1)
-  io.deq.valid := validAfterInv(0)
+  // We don't want to invalidate deq.valid response right away even when
+  // io.invalidate(head) is true.
+  // Coalescing unit consumes queue head's validity, and produces its new
+  // validity.  Deasserting deq.valid right away will result in a combinational
+  // cycle.
+  io.deq.valid := valid(0)
   io.deq.bits := elts.head
 
   assert(!flow, "flow-through is not implemented")
@@ -540,19 +546,6 @@ class CoalShiftQueue[T <: Data](
   io.count := PopCount(io.mask)
 }
 
-object CoalShiftQueue {
-  def apply[T <: Data](
-      enq: DecoupledIO[T],
-      entries: Int = 2,
-      pipe: Boolean = false,
-      flow: Boolean = false
-  ): DecoupledIO[T] = {
-    val q = Module(new CoalShiftQueue(enq.bits.cloneType, entries, pipe, flow))
-    q.io.enq <> enq
-    q.io.deq
-  }
-}
-
 class MemTraceDriver(numLanes: Int = 1)(implicit p: Parameters) extends LazyModule {
   // Create N client nodes together
   val laneNodes = Seq.tabulate(numLanes) { i =>
diff --git a/src/test/scala/CoalescingUnitTest.scala b/src/test/scala/CoalescingUnitTest.scala
index 0cca2ea..8369eb2 100644
--- a/src/test/scala/CoalescingUnitTest.scala
+++ b/src/test/scala/CoalescingUnitTest.scala
@@ -111,7 +111,7 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
     }
   }
 
-  it should "invalidate entry being dequeued combinationally" in {
+  it should "invalidate head being dequeued" in {
     test(new CoalShiftQueue(UInt(8.W), 4)) { c =>
       c.io.invalidate.poke(0.U)
 
@@ -128,13 +128,14 @@ class CoalShiftQueueTest extends AnyFlatSpec with ChiselScalatestTester {
       c.clock.step()
       c.io.enq.valid.poke(false.B)
 
-      // invalidate should work for the entry just being dequeued at the same
-      // cycle
+      // invalidate should work for the head just being dequeued at the same
+      // cycle.  However, it should not change deq.valid right away to avoid
+      // combinational cycles (see definition).
       c.io.invalidate.poke(0x1.U)
       c.io.deq.ready.poke(true.B)
-      c.io.deq.valid.expect(false.B)
+      c.io.deq.valid.expect(true.B)
       c.clock.step()
-      // rest are unchanged
+      // 0x12 should have been dequeued
       c.io.invalidate.poke(0.U)
       c.io.deq.ready.poke(true.B)
       c.io.deq.valid.expect(true.B)