Implement RFC 20: Remove non-FWFT FIFOs.

wanda-phi · whitequark · commit 8cd8cdde2b62 · 2023-12-13T11:41:19.000Z
Fixes #875.
diff --git a/amaranth/lib/fifo.py b/amaranth/lib/fifo.py
@@ -55,31 +55,21 @@ class FIFOInterface:
     read at the output interface (``r_data``, ``r_rdy``, ``r_en``). The data entry written first
     to the input also appears first on the output.
     """,
-    parameters="""
-    fwft : bool
-        First-word fallthrough. If set, when ``r_rdy`` rises, the first entry is already
-        available, i.e. ``r_data`` is valid. Otherwise, after ``r_rdy`` rises, it is necessary
-        to strobe ``r_en`` for ``r_data`` to become valid.
-    """.strip(),
+    parameters="",
     r_data_valid="The conditions in which ``r_data`` is valid depends on the type of the queue.",
     attributes="",
     w_attributes="",
     r_attributes="")
 
-    def __init__(self, *, width, depth, fwft=True):
+    def __init__(self, *, width, depth):
         if not isinstance(width, int) or width < 0:
             raise TypeError("FIFO width must be a non-negative integer, not {!r}"
                             .format(width))
         if not isinstance(depth, int) or depth < 0:
             raise TypeError("FIFO depth must be a non-negative integer, not {!r}"
                             .format(depth))
-        if not fwft:
-            warnings.warn("support for FIFOs with `fwft=False` will be removed without a replacement; "
-                          "consider switching to `fwft=True` or copy the module into your project to continue using it",
-                          DeprecationWarning)
         self.width = width
         self.depth = depth
-        self.fwft  = fwft
 
         self.w_data = Signal(width, reset_less=True)
         self.w_rdy  = Signal() # writable; not full
@@ -107,29 +97,17 @@ class SyncFIFO(Elaboratable, FIFOInterface):
     Read and write interfaces are accessed from the same clock domain. If different clock domains
     are needed, use :class:`AsyncFIFO`.
     """.strip(),
-    parameters="""
-    fwft : bool
-        First-word fallthrough. If set, when the queue is empty and an entry is written into it,
-        that entry becomes available on the output on the same clock cycle. Otherwise, it is
-        necessary to assert ``r_en`` for ``r_data`` to become valid.
-    """.strip(),
-    r_data_valid="For FWFT queues, valid if ``r_rdy`` is asserted. "
-                 "For non-FWFT queues, valid on the next cycle after ``r_rdy`` and ``r_en`` have been asserted.",
+    parameters="",
+    r_data_valid="Valid if ``r_rdy`` is asserted.",
     attributes="""
     level : Signal(range(depth + 1)), out
         Number of unread entries. This level is the same between read and write for synchronous FIFOs.
     """.strip(),
     r_attributes="",
     w_attributes="")
 
-    def __init__(self, *, width, depth, fwft=True):
-        if not fwft:
-            warnings.warn("support for FIFOs with `fwft=False` will be removed without a replacement; "
-                          "consider switching to `fwft=True` or copy the module into your project to continue using it",
-                          DeprecationWarning)
+    def __init__(self, *, width, depth):
         super().__init__(width=width, depth=depth)
-        # Fix up fwft after initialization to avoid the warning from FIFOInterface.
-        self.fwft = fwft
 
         self.level = Signal(range(depth + 1))
 
@@ -154,8 +132,7 @@ def elaborate(self, platform):
 
         storage = Memory(width=self.width, depth=self.depth)
         w_port  = m.submodules.w_port = storage.write_port()
-        r_port  = m.submodules.r_port = storage.read_port(
-            domain="comb" if self.fwft else "sync", transparent=self.fwft)
+        r_port  = m.submodules.r_port = storage.read_port(domain="comb")
         produce = Signal(range(self.depth))
         consume = Signal(range(self.depth))
 
@@ -171,8 +148,6 @@ def elaborate(self, platform):
             r_port.addr.eq(consume),
             self.r_data.eq(r_port.data),
         ]
-        if not self.fwft:
-            m.d.comb += r_port.en.eq(self.r_en)
         with m.If(do_read):
             m.d.sync += consume.eq(_incr(consume, self.depth))
 
@@ -214,16 +189,13 @@ class SyncFIFOBuffered(Elaboratable, FIFOInterface):
     description="""
     Buffered synchronous first in, first out queue.
 
-    This queue's interface is identical to :class:`SyncFIFO` configured as ``fwft=True``, but it
+    This queue's interface is identical to :class:`SyncFIFO`, but it
     does not use asynchronous memory reads, which are incompatible with FPGA block RAMs.
 
     In exchange, the latency between an entry being written to an empty queue and that entry
     becoming available on the output is increased by one cycle compared to :class:`SyncFIFO`.
     """.strip(),
-    parameters="""
-    fwft : bool
-        Always set.
-    """.strip(),
+    parameters="",
     attributes="""
     level : Signal(range(depth + 1)), out
         Number of unread entries. This level is the same between read and write for synchronous FIFOs.
@@ -369,8 +341,6 @@ class AsyncFIFO(Elaboratable, FIFOInterface):
         Read clock domain.
     w_domain : str
         Write clock domain.
-    fwft : bool
-        Always set.
     """.strip(),
     attributes="",
     r_data_valid="Valid if ``r_rdy`` is asserted.",
@@ -548,8 +518,6 @@ class AsyncFIFOBuffered(Elaboratable, FIFOInterface):
         Read clock domain.
     w_domain : str
         Write clock domain.
-    fwft : bool
-        Always set.
     """.strip(),
     attributes="",
     r_data_valid="Valid if ``r_rdy`` is asserted.",
diff --git a/docs/changes.rst b/docs/changes.rst
@@ -25,6 +25,8 @@ Standard library changes
 .. currentmodule:: amaranth.lib
 
 * Removed: (deprecated in 0.4) :mod:`amaranth.lib.scheduler`. (`RFC 19`_)
+* Removed: (deprecated in 0.4) :class:`amaranth.lib.fifo.FIFOInterface` with ``fwft=False``. (`RFC 20`_)
+* Removed: (deprecated in 0.4) :class:`amaranth.lib.fifo.SyncFIFO` with ``fwft=False``. (`RFC 20`_)
 
 
 Platform integration changes
diff --git a/docs/stdlib/fifo.rst b/docs/stdlib/fifo.rst
@@ -12,7 +12,7 @@ The ``amaranth.lib.fifo`` module provides building blocks for first-in, first-ou
 
       The :class:`FIFOInterface` class can be used directly to substitute a FIFO in tests, or inherited from in a custom FIFO implementation.
 
-.. autoclass:: SyncFIFO(*, width, depth, fwft=True)
+.. autoclass:: SyncFIFO(*, width, depth)
 .. autoclass:: SyncFIFOBuffered(*, width, depth)
 .. autoclass:: AsyncFIFO(*, width, depth, r_domain="read", w_domain="write", exact_depth=False)
 .. autoclass:: AsyncFIFOBuffered(*, width, depth, r_domain="read", w_domain="write", exact_depth=False)
diff --git a/tests/test_lib_fifo.py b/tests/test_lib_fifo.py
@@ -15,10 +15,10 @@ class FIFOTestCase(FHDLTestCase):
     def test_depth_wrong(self):
         with self.assertRaisesRegex(TypeError,
                 r"^FIFO width must be a non-negative integer, not -1$"):
-            FIFOInterface(width=-1, depth=8, fwft=True)
+            FIFOInterface(width=-1, depth=8)
         with self.assertRaisesRegex(TypeError,
                 r"^FIFO depth must be a non-negative integer, not -1$"):
-            FIFOInterface(width=8, depth=-1, fwft=True)
+            FIFOInterface(width=8, depth=-1)
 
     def test_sync_depth(self):
         self.assertEqual(SyncFIFO(width=8, depth=0).depth, 0)
@@ -68,8 +68,8 @@ class FIFOModel(Elaboratable, FIFOInterface):
     """
     Non-synthesizable first-in first-out queue, implemented naively as a chain of registers.
     """
-    def __init__(self, *, width, depth, fwft, r_domain, w_domain):
-        super().__init__(width=width, depth=depth, fwft=fwft)
+    def __init__(self, *, width, depth, r_domain, w_domain):
+        super().__init__(width=width, depth=depth)
 
         self.r_domain = r_domain
         self.w_domain = w_domain
@@ -90,11 +90,8 @@ def elaborate(self, platform):
 
         m.d.comb += self.r_rdy.eq(self.level > 0)
         m.d.comb += r_port.addr.eq((consume + 1) % self.depth)
-        if self.fwft:
-            m.d.comb += self.r_data.eq(r_port.data)
+        m.d.comb += self.r_data.eq(r_port.data)
         with m.If(self.r_en & self.r_rdy):
-            if not self.fwft:
-                m.d[self.r_domain] += self.r_data.eq(r_port.data)
             m.d[self.r_domain] += consume.eq(r_port.addr)
 
         m.d.comb += self.w_rdy.eq(self.level < self.depth)
@@ -136,7 +133,7 @@ def __init__(self, fifo, r_domain, w_domain):
     def elaborate(self, platform):
         m = Module()
         m.submodules.dut  = dut  = self.fifo
-        m.submodules.gold = gold = FIFOModel(width=dut.width, depth=dut.depth, fwft=dut.fwft,
+        m.submodules.gold = gold = FIFOModel(width=dut.width, depth=dut.depth,
                                              r_domain=self.r_domain, w_domain=self.w_domain)
 
         m.d.comb += [
@@ -150,19 +147,7 @@ def elaborate(self, platform):
         m.d.comb += Assert(dut.r_level == gold.r_level)
         m.d.comb += Assert(dut.w_level == gold.w_level)
 
-        if dut.fwft:
-            m.d.comb += Assert(dut.r_rdy
-                               .implies(dut.r_data == gold.r_data))
-        else:
-            # past_dut_r_rdy = Past(dut.r_rdy, domain=self.r_domain)
-            past_dut_r_rdy = Signal.like(dut.r_rdy, attrs={"amaranth.sample_reg": True})
-            m.d[self.r_domain] += past_dut_r_rdy.eq(dut.r_rdy)
-
-            # past_dut_r_en = Past(dut.r_en, domain=self.r_domain)
-            past_dut_r_en = Signal.like(dut.r_en, attrs={"amaranth.sample_reg": True})
-            m.d[self.r_domain] += past_dut_r_en.eq(dut.r_en)
-
-            m.d.comb += Assert((past_dut_r_rdy & past_dut_r_en).implies(dut.r_data == gold.r_data))
+        m.d.comb += Assert(dut.r_rdy.implies(dut.r_data == gold.r_data))
 
         return m
 
@@ -219,12 +204,7 @@ def elaborate(self, platform):
             read_2 = Signal(fifo.width)
             with m.State("READ"):
                 m.d.comb += fifo.r_en.eq(1)
-                if fifo.fwft:
-                    r_rdy = fifo.r_rdy
-                else: # r_rdy = Past(fifo.r_rdy, domain=self.r_domain)
-                    r_rdy = Signal.like(fifo.r_rdy, attrs={"amaranth.sample_reg": True})
-                    m.d[self.r_domain] += r_rdy.eq(fifo.r_rdy)
-                with m.If(r_rdy):
+                with m.If(fifo.r_rdy):
                     m.d.sync += [
                         read_1.eq(read_2),
                         read_2.eq(fifo.r_data),
@@ -271,21 +251,11 @@ def check_sync_fifo(self, fifo):
                                            bound=fifo.depth * 2 + 1),
                           mode="hybrid", depth=fifo.depth * 2 + 1)
 
-    def test_sync_fwft_pot(self):
-        self.check_sync_fifo(SyncFIFO(width=8, depth=4, fwft=True))
-
-    def test_sync_fwft_npot(self):
-        self.check_sync_fifo(SyncFIFO(width=8, depth=5, fwft=True))
-
-    def test_sync_not_fwft_pot(self):
-        with warnings.catch_warnings():
-            warnings.filterwarnings(action="ignore", category=DeprecationWarning)
-            self.check_sync_fifo(SyncFIFO(width=8, depth=4, fwft=False))
+    def test_sync_pot(self):
+        self.check_sync_fifo(SyncFIFO(width=8, depth=4))
 
-    def test_sync_not_fwft_npot(self):
-        with warnings.catch_warnings():
-            warnings.filterwarnings(action="ignore", category=DeprecationWarning)
-            self.check_sync_fifo(SyncFIFO(width=8, depth=5, fwft=False))
+    def test_sync_npot(self):
+        self.check_sync_fifo(SyncFIFO(width=8, depth=5))
 
     def test_sync_buffered_pot(self):
         self.check_sync_fifo(SyncFIFOBuffered(width=8, depth=4))
