From 64856fdb7e2f615a9f55b7325b74f82bf769da83 Mon Sep 17 00:00:00 2001 From: Wanda Date: Fri, 20 Oct 2023 19:53:06 +0200 Subject: [PATCH 1/3] lib.fifo: make `fwft=True` the default --- amaranth/lib/fifo.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/amaranth/lib/fifo.py b/amaranth/lib/fifo.py index 4fd6f6ccc..8c6b6b349 100644 --- a/amaranth/lib/fifo.py +++ b/amaranth/lib/fifo.py @@ -64,7 +64,7 @@ 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)) @@ -221,7 +221,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)) @@ -306,7 +306,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 +483,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 From 80fd07653e6807dda9c856de7f99abce10ba1848 Mon Sep 17 00:00:00 2001 From: Wanda Date: Tue, 24 Oct 2023 21:46:14 +0200 Subject: [PATCH 2/3] lib.fifo: reimplement `SyncFIFOBuffered` without inner `SyncFIFO`. --- amaranth/lib/fifo.py | 100 +++++++++++++++++++++++++++++++++++------ tests/test_lib_fifo.py | 3 ++ 2 files changed, 90 insertions(+), 13 deletions(-) diff --git a/amaranth/lib/fifo.py b/amaranth/lib/fifo.py index 8c6b6b349..7cf071f3b 100644 --- a/amaranth/lib/fifo.py +++ b/amaranth/lib/fifo.py @@ -234,32 +234,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 diff --git a/tests/test_lib_fifo.py b/tests/test_lib_fifo.py index 9e7427952..58cf1d429 100644 --- a/tests/test_lib_fifo.py +++ b/tests/test_lib_fifo.py @@ -270,6 +270,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. From c68bed07fdbe95af5e37758323237ea543860cbd Mon Sep 17 00:00:00 2001 From: Wanda Date: Tue, 24 Oct 2023 21:55:51 +0200 Subject: [PATCH 3/3] Implement RFC 20: Deprecate non-FWFT FIFOs. Tracking issue #875. --- amaranth/lib/fifo.py | 16 ++++++++++++++-- docs/changes.rst | 5 +++++ tests/test_lib_fifo.py | 10 ++++++++-- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/amaranth/lib/fifo.py b/amaranth/lib/fifo.py index 7cf071f3b..34c3b1049 100644 --- a/amaranth/lib/fifo.py +++ b/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 @@ -71,6 +73,10 @@ def __init__(self, *, width, depth, fwft=True): 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)) @@ -326,7 +338,7 @@ def elaborate(self, platform): 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): diff --git a/docs/changes.rst b/docs/changes.rst index 6986ddb0f..c982a5f26 100644 --- a/docs/changes.rst +++ b/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 diff --git a/tests/test_lib_fifo.py b/tests/test_lib_fifo.py index 58cf1d429..e6e1caf77 100644 --- a/tests/test_lib_fifo.py +++ b/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))