Implement RFC 20: Deprecate non-FWFT FIFOs.

amaranth/lib/fifo.py

@@ -1,5 +1,7 @@
 """First-in first-out queues."""
 
+import warnings
+
 from .. import *
 from ..asserts import *
 from .._utils import log2_int
@@ -64,13 +66,17 @@ class FIFOInterface:
     w_attributes="",
     r_attributes="")
 
-    def __init__(self, *, width, depth, fwft):
+    def __init__(self, *, width, depth, fwft=True):
         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
@@ -117,7 +123,13 @@ class SyncFIFO(Elaboratable, FIFOInterface):
     w_attributes="")
 
     def __init__(self, *, width, depth, fwft=True):
-        super().__init__(width=width, depth=depth, fwft=fwft)
+        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)
+        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))
 
@@ -221,7 +233,7 @@ class SyncFIFOBuffered(Elaboratable, FIFOInterface):
     w_attributes="")
 
     def __init__(self, *, width, depth):
-        super().__init__(width=width, depth=depth, fwft=True)
+        super().__init__(width=width, depth=depth)
 
         self.level = Signal(range(depth + 1))
 
@@ -234,32 +246,106 @@ def elaborate(self, platform):
             ]
             return m
 
-        # Effectively, this queue treats the output register of the non-FWFT inner queue as
-        # an additional storage element.
-        m.submodules.unbuffered = fifo = SyncFIFO(width=self.width, depth=self.depth - 1,
-                                                  fwft=False)
+        do_write = self.w_rdy & self.w_en
+        do_read = self.r_rdy & self.r_en
 
         m.d.comb += [
-            fifo.w_data.eq(self.w_data),
-            fifo.w_en.eq(self.w_en),
-            self.w_rdy.eq(fifo.w_rdy),
+            self.w_level.eq(self.level),
+            self.r_level.eq(self.level),
         ]
 
+        if self.depth == 1:
+            # Special case: a single register. Note that, by construction, this will
+            # only be able to push a value every other cycle (alternating between
+            # full and empty).
+            m.d.comb += [
+                self.w_rdy.eq(self.level == 0),
+                self.r_rdy.eq(self.level == 1),
+            ]
+            with m.If(do_write):
+                m.d.sync += [
+                    self.r_data.eq(self.w_data),
+                    self.level.eq(1),
+                ]
+            with m.If(do_read):
+                m.d.sync += [
+                    self.level.eq(0),
+                ]
+
+            return m
+
+        inner_depth = self.depth - 1
+        inner_level = Signal(range(inner_depth + 1))
+        inner_r_rdy = Signal()
+
+        m.d.comb += [
+            self.w_rdy.eq(inner_level != inner_depth),
+            inner_r_rdy.eq(inner_level != 0),
+        ]
+
+        do_inner_read  = inner_r_rdy & (~self.r_rdy | self.r_en)
+
+        m.submodules.storage = storage = Memory(width=self.width, depth=inner_depth)
+        w_port  = storage.write_port()
+        r_port  = storage.read_port(domain="sync", transparent=False)
+        produce = Signal(range(inner_depth))
+        consume = Signal(range(inner_depth))
+
         m.d.comb += [
-            self.r_data.eq(fifo.r_data),
-            fifo.r_en.eq(fifo.r_rdy & (~self.r_rdy | self.r_en)),
+            w_port.addr.eq(produce),
+            w_port.data.eq(self.w_data),
+            w_port.en.eq(do_write),
         ]
-        with m.If(fifo.r_en):
+        with m.If(do_write):
+            m.d.sync += produce.eq(_incr(produce, inner_depth))
+
+        m.d.comb += [
+            r_port.addr.eq(consume),
+            self.r_data.eq(r_port.data),
+            r_port.en.eq(do_inner_read)
+        ]
+        with m.If(do_inner_read):
+            m.d.sync += consume.eq(_incr(consume, inner_depth))
+
+        with m.If(do_write & ~do_inner_read):
+            m.d.sync += inner_level.eq(inner_level + 1)
+        with m.If(do_inner_read & ~do_write):
+            m.d.sync += inner_level.eq(inner_level - 1)
+
+        with m.If(do_inner_read):
             m.d.sync += self.r_rdy.eq(1)
         with m.Elif(self.r_en):
             m.d.sync += self.r_rdy.eq(0)
 
         m.d.comb += [
-            self.level.eq(fifo.level + self.r_rdy),
-            self.w_level.eq(self.level),
-            self.r_level.eq(self.level),
+            self.level.eq(inner_level + self.r_rdy),
         ]
 
+        if platform == "formal":
+            # TODO: move this logic to SymbiYosys
+            with m.If(Initial()):
+                m.d.comb += [
+                    Assume(produce < inner_depth),
+                    Assume(consume < inner_depth),
+                ]
+                with m.If(produce == consume):
+                    m.d.comb += Assume((inner_level == 0) | (inner_level == inner_depth))
+                with m.If(produce > consume):
+                    m.d.comb += Assume(inner_level == (produce - consume))
+                with m.If(produce < consume):
+                    m.d.comb += Assume(inner_level == (inner_depth + produce - consume))
+            with m.Else():
+                m.d.comb += [
+                    Assert(produce < inner_depth),
+                    Assert(consume < inner_depth),
+                ]
+                with m.If(produce == consume):
+                    m.d.comb += Assert((inner_level == 0) | (inner_level == inner_depth))
+                with m.If(produce > consume):
+                    m.d.comb += Assert(inner_level == (produce - consume))
+                with m.If(produce < consume):
+                    m.d.comb += Assert(inner_level == (inner_depth + produce - consume))
+
         return m
 
 
@@ -306,7 +392,7 @@ def __init__(self, *, width, depth, r_domain="read", w_domain="write", exact_dep
                                  .format(depth)) from None
         else:
             depth_bits = 0
-        super().__init__(width=width, depth=depth, fwft=True)
+        super().__init__(width=width, depth=depth)
 
         self.r_rst = Signal()
         self._r_domain = r_domain
@@ -483,7 +569,7 @@ def __init__(self, *, width, depth, r_domain="read", w_domain="write", exact_dep
                 raise ValueError("AsyncFIFOBuffered only supports depths that are one higher "
                                  "than powers of 2; requested exact depth {} is not"
                                  .format(depth)) from None
-        super().__init__(width=width, depth=depth, fwft=True)
+        super().__init__(width=width, depth=depth)
 
         self.r_rst = Signal()
         self._r_domain = r_domain

docs/changes.rst

@@ -33,6 +33,7 @@ Apply the following changes to code written against Amaranth 0.3 to migrate it t
 * Replace uses of ``Record`` with :mod:`amaranth.lib.data` and :mod:`amaranth.lib.wiring`. The appropriate replacement depends on the use case. If ``Record`` was being used for data storage and accessing the bit-level representation, use :mod:`amaranth.lib.data`. If ``Record`` was being used for connecting design components together, use :mod:`amaranth.lib.wiring`.
 * Ensure the ``Pin`` instance returned by ``platform.request`` is not cast to value directly, but used for its fields. Replace code like ``leds = Cat(platform.request(led, n) for n in range(4))`` with ``leds = Cat(platform.request(led, n).o for n in range(4))`` (note the ``.o``).
 * Remove uses of ``amaranth.lib.scheduler.RoundRobin`` by inlining or copying the implementation of that class.
+* Remove uses of ``amaranth.lib.fifo.SyncFIFO(fwft=False)`` and ``amaranth.lib.fifo.FIFOInterface(fwft=False)`` by converting code to use ``fwft=True`` FIFOs or copying the implementation of those classes.
 
 While code that uses the features listed as deprecated below will work in Amaranth 0.4, they will be removed in the next version.
 
@@ -52,6 +53,7 @@ Implemented RFCs
 .. _RFC 15: https://amaranth-lang.org/rfcs/0015-lifting-shape-castables.html
 .. _RFC 18: https://amaranth-lang.org/rfcs/0018-reorganize-vendor-platforms.html
 .. _RFC 19: https://amaranth-lang.org/rfcs/0019-remove-scheduler.html
+.. _RFC 20: https://amaranth-lang.org/rfcs/0020-deprecate-non-fwft-fifos.html
 .. _RFC 22: https://amaranth-lang.org/rfcs/0022-valuecastable-shape.html
 
 
@@ -67,6 +69,7 @@ Implemented RFCs
 * `RFC 18`_: Reorganize vendor platforms
 * `RFC 19`_: Remove ``amaranth.lib.scheduler``
 * `RFC 15`_: Lifting shape-castable objects
+* `RFC 20`_: Deprecate non-FWFT FIFOs
 * `RFC 22`_: Define ``ValueCastable.shape()``
 
 
@@ -103,6 +106,8 @@ Standard library changes
 * Added: :mod:`amaranth.lib.data`. (`RFC 1`_)
 * Added: :mod:`amaranth.lib.crc`. (`RFC 6`_)
 * Deprecated: :mod:`amaranth.lib.scheduler`. (`RFC 19`_)
+* Deprecated: :class:`amaranth.lib.fifo.FIFOInterface` with ``fwft=False``. (`RFC 20`_)
+* Deprecated: :class:`amaranth.lib.fifo.SyncFIFO` with ``fwft=False``. (`RFC 20`_)
 
 
 Toolchain changes

tests/test_lib_fifo.py

@@ -1,5 +1,7 @@
 # amaranth: UnusedElaboratable=no
 
+import warnings
+
 from amaranth.hdl import *
 from amaranth.asserts import *
 from amaranth.sim import *
@@ -256,10 +258,14 @@ def test_sync_fwft_npot(self):
         self.check_sync_fifo(SyncFIFO(width=8, depth=5, fwft=True))
 
     def test_sync_not_fwft_pot(self):
-        self.check_sync_fifo(SyncFIFO(width=8, depth=4, fwft=False))
+        with warnings.catch_warnings():
+            warnings.filterwarnings(action="ignore", category=DeprecationWarning)
+            self.check_sync_fifo(SyncFIFO(width=8, depth=4, fwft=False))
 
     def test_sync_not_fwft_npot(self):
-        self.check_sync_fifo(SyncFIFO(width=8, depth=5, fwft=False))
+        with warnings.catch_warnings():
+            warnings.filterwarnings(action="ignore", category=DeprecationWarning)
+            self.check_sync_fifo(SyncFIFO(width=8, depth=5, fwft=False))
 
     def test_sync_buffered_pot(self):
         self.check_sync_fifo(SyncFIFOBuffered(width=8, depth=4))
@@ -270,6 +276,9 @@ def test_sync_buffered_potp1(self):
     def test_sync_buffered_potm1(self):
         self.check_sync_fifo(SyncFIFOBuffered(width=8, depth=3))
 
+    def test_sync_buffered_one(self):
+        self.check_sync_fifo(SyncFIFOBuffered(width=8, depth=1))
+
     def check_async_fifo(self, fifo):
         # TODO: properly doing model equivalence checking on this likely requires multiclock,
         # which is not really documented nor is it clear how to use it.