Repository: amaranth-lang/amaranth (python)

Issue:
Inferring True Dual-Port BRAMs with the new IR on Series 7 and ECP5
I'm trying to infer a true dual port BRAM on a Xilinx Series 7 chip, but I'm not having much luck. I've written the following to try to map the ReadPort/WritePort construct to the addr/din/dout/en/wea that's native to the RAM36 primitives:

```python
from amaranth import *
from amaranth_boards.nexys4ddr import Nexys4DDRPlatform
from amaranth.sim import Simulator
from random import randint

class TrueDualPort(Elaboratable):
    def __init__(self, width, depth):
        self.mem = Memory(
            width=width,
            depth=depth,
            init = [randint(0, 2**width-1) for _ in range(depth)])

        self.addra = Signal(range(depth))
        self.dina = Signal(width)
        self.douta = Signal(width)
        self.wea = Signal()
        self.ena = Signal()

        self.addrb = Signal(range(depth))
        self.dinb = Signal(width)
        self.doutb = Signal(width)
        self.web = Signal()
        self.enb = Signal()

    def elaborate(self, platform):
        m = Module()

        m.submodules["mem"] = self.mem
        rp0 = self.mem.read_port()
        wp0 = self.mem.write_port()

        rp1 = self.mem.read_port()
        wp1 = self.mem.write_port()

        m.d.comb += rp0.addr.eq(self.addra)
        m.d.comb += wp0.addr.eq(self.addra)
        m.d.comb += wp0.data.eq(self.dina)
        m.d.comb += self.douta.eq(rp0.data)
        m.d.comb += rp0.en.eq(self.ena)
        m.d.comb += wp0.en.eq(self.wea & self.ena)

        m.d.comb += rp1.addr.eq(self.addrb)
        m.d.comb += wp1.addr.eq(self.addrb)
        m.d.comb += wp1.data.eq(self.dinb)
        m.d.comb += self.doutb.eq(rp1.data)
        m.d.comb += rp1.en.eq(self.enb)
        m.d.comb += wp1.en.eq(self.web & self.enb)
        return m
```

And if I create a design in the following format, Vivado will recognize it as a True Dual Port BRAM. I'm building for the Nexys4DDR, which has 16 switches and 16 LEDs, and I'm using the top half of each for Port A, and the bottom half for Port B, just to prevent the memory from being optimized out.

```python
class TrueDualPortTest(Elaboratable):
    def elaborate(self, platform):
        m = Module()
        m.submodules["tdp"] = tdp = TrueDualPort(8, 4096)

        sw_pins = Cat([platform.request("switch",i).i for i in range(8)])
        led_pins = Cat([platform.request("led",i).o for i in range(8)])

        counter = Signal(8)
        m.d.sync += counter.eq(counter + 1)

        # ---- Port A ----
        m.d.comb += tdp.ena.eq(1)
        m.d.comb += tdp.wea.eq(1)
        m.d.sync += tdp.dina.eq(counter) # <- uncommenting this makes Vivado unable to recognize it as a TDP!
        m.d.sync += tdp.addra.eq(sw_pins[8:])
        m.d.sync += led_pins[8:].eq(tdp.douta)


        # ---- Port B ----
        m.d.comb += tdp.enb.eq(1)
        m.d.comb += tdp.web.eq(0)
        m.d.sync += tdp.dinb.eq(counter + 3) #  <- uncommenting this makes Vivado unable to recognize it as a TDP!
        m.d.sync += tdp.addrb.eq(sw_pins[:8])
        m.d.sync += led_pins[:8].eq(tdp.doutb)

        return m
```

If I build this with `Nexys4DDRPlatform().build(TrueDualPortTest())`, then Vivado will happily recognize this as a true dual port BRAM, as is seen in the logs:

`INFO: [Synth 8-3971] The signal "\top.tdp :/mem_reg" was recognized as a true dual port RAM template.`

And the post-placement resource utilization report will show that block ram is being used. However, if I uncomment either (or both) of the two lines marked in the source above - Vivado will map everything to distributed RAM instead. This seems to hold true if I keep increasing the depth of the RAM. 

I also noticed that if I assign the `en` and `we` signals synchronously instead of combinationally, Vivado will throw an `Unrecognized RAM template` error.

My question is: How do I create a true-dual port RAM in Amaranth? I'd like to avoid directly instantiating an inferred BRAM template, since I'm not able to use the built-in simulator on external Verilog.

Thanks everyone!


Related Info:

I did notice that a while back the Yosys memory interface was reworked and support for TDP memories was added - I'm not sure if that has any implications for Amaranth, though.
https://github.com/YosysHQ/yosys/issues/1959

And that appears to be reflected in the yosys docs:
https://yosyshq.readthedocs.io/projects/yosys/en/latest/CHAPTER_Memorymap.html


Pull request:
back.rtlil: set read port init to all-x.
This is an unfortunate necessity needed to fix memory inference regressions introduced when we switched to using v2 cells. A better approach, compatible with RFC 54, will need to be figured out for Amaranth 0.6.

Fixes #1011.

Patch:
diff --git a/amaranth/back/rtlil.py b/amaranth/back/rtlil.py
index 8c6ff22..cecb11f 100644
--- a/amaranth/back/rtlil.py
+++ b/amaranth/back/rtlil.py
@@ -20,6 +20,12 @@ _escape_map = str.maketrans({
 })
 
 
+# Special hack to emit 'x consts for memory read port init.
+class Undef:
+    def __init__(self, width):
+        self.width = width
+
+
 def _signed(value):
     if isinstance(value, str):
         return False
@@ -27,6 +33,8 @@ def _signed(value):
         return value < 0
     elif isinstance(value, _ast.Const):
         return value.shape().signed
+    elif isinstance(value, Undef):
+        return False
     else:
         assert False, f"Invalid constant {value!r}"
 
@@ -45,6 +53,8 @@ def _const(value):
     elif isinstance(value, _ast.Const):
         value_twos_compl = value.value & ((1 << len(value)) - 1)
         return "{}'{:0{}b}".format(len(value), value_twos_compl, len(value))
+    elif isinstance(value, Undef):
+        return f"{value.width}'" + "x" * value.width
     else:
         assert False, f"Invalid constant {value!r}"
 
@@ -1070,9 +1080,27 @@ class ModuleEmitter:
             "WIDTH": cell.width,
             "TRANSPARENCY_MASK": _ast.Const(transparency_mask, memory_info.num_write_ports),
             "COLLISION_X_MASK": _ast.Const(0, memory_info.num_write_ports),
-            "ARST_VALUE": _ast.Const(0, cell.width),
-            "SRST_VALUE": _ast.Const(0, cell.width),
-            "INIT_VALUE": _ast.Const(0, cell.width),
+            # Horrible hack alert: Yosys has two different Verilog code patterns it can emit for
+            # transparent synchronous read ports — the old, limitted one and the generic new one.
+            # The old one essentially consists of a combinational read port with a register added
+            # on the *address* input. It has several limitations:
+            #
+            # - can only express read ports transparent wrt *all* write ports
+            # - cannot support initial values
+            # - cannot support reset
+            # - cannot support clock enable
+            #
+            # The new pattern can express any supported read port, but is not widely recognized
+            # by other toolchains, leading to memory inference failures. Thus, Yosys will use
+            # the old pattern whenever possible.
+            #
+            # In order to enable Yosys to use the old pattern and avoid memory inference regressions
+            # with non-Yosys synthesis, we need to emit undefined initial value here. This is in
+            # direct conflict with RFC 54, and will have to be revisited before 0.6, possibly
+            # requiring a large-scale design change in Amaranth memory support.
+            "ARST_VALUE": Undef(cell.width),
+            "SRST_VALUE": Undef(cell.width),
+            "INIT_VALUE": Undef(cell.width),
             "CE_OVER_SRST": False,
         }
         if isinstance(cell, _nir.AsyncReadPort):


Test patch:
diff --git a/tests/test_back_rtlil.py b/tests/test_back_rtlil.py
index 96ac2ee..6c4dbcc 100644
--- a/tests/test_back_rtlil.py
+++ b/tests/test_back_rtlil.py
@@ -1643,9 +1643,9 @@ class MemoryTestCase(RTLILTestCase):
                 parameter \WIDTH 8
                 parameter \TRANSPARENCY_MASK 1'0
                 parameter \COLLISION_X_MASK 1'0
-                parameter \ARST_VALUE 8'00000000
-                parameter \SRST_VALUE 8'00000000
-                parameter \INIT_VALUE 8'00000000
+                parameter \ARST_VALUE 8'xxxxxxxx
+                parameter \SRST_VALUE 8'xxxxxxxx
+                parameter \INIT_VALUE 8'xxxxxxxx
                 parameter \CE_OVER_SRST 0
                 parameter \CLK_ENABLE 0
                 parameter \CLK_POLARITY 1
@@ -1725,9 +1725,9 @@ class MemoryTestCase(RTLILTestCase):
                 parameter \WIDTH 8
                 parameter \TRANSPARENCY_MASK 1'1
                 parameter \COLLISION_X_MASK 1'0
-                parameter \ARST_VALUE 8'00000000
-                parameter \SRST_VALUE 8'00000000
-                parameter \INIT_VALUE 8'00000000
+                parameter \ARST_VALUE 8'xxxxxxxx
+                parameter \SRST_VALUE 8'xxxxxxxx
+                parameter \INIT_VALUE 8'xxxxxxxx
                 parameter \CE_OVER_SRST 0
                 parameter \CLK_ENABLE 1
                 parameter \CLK_POLARITY 1