search

amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.54k stars 170 forks source link

Memory generates an empty Verilog wrport module that Vivado considers a black box #899

Closed daniestevez closed 1 year ago

daniestevez commented 1 year ago

I've noticed that after updating to the current https://github.com/amaranth-lang/amaranth/commit/f135226a79fddf5caf894030ac7f49995774c3a7 main branch of amaranth, and OSS CAD suite 20230903 (relevant, since the problem might actually be in Yosys), my amaranth code is failing to build on Vivado.

This is the code in question. It uses an amaranth Memory with rdport and wrport. These new versions of amaranth and Yosis generate some empty modules that look like this

(* \amaranth.hierarchy  = "top.spectrometer.fft.bfly0.bfly0.wrport" *)
(* generator = "Amaranth" *)
module wrport();
  wire \$empty_module_filler ;
endmodule

and which get instantiated in the module (\bfly0$1) that contains the Memory. The older versions of amaranth and Yosis that I was using before, which are from 2023-06-10, don't do this. They don't generate the empty wrport modules, and in fact the module \bfly0$1 they generate doesn't instantiate any modules.

Vivado is failing to build due to these empty modules. The error it gives is

ERROR: [DRC INBB-3] Black Box Instances: Cell 'i_system_wrapper/system_i/maia_sdr/inst/spectrometer/fft/bfly0/bfly0/wrport' of type 'system_maia_sdr_0_wrport' has undefined contents and is considered a black box.  The contents of this cell must be defined for opt_design to complete successfully.

so apparently the wire \$empty_module_filler is not enough to make Vivado not consider these modules as black boxes.

More information (probably not so helpful) in https://github.com/maia-sdr/maia-sdr/pull/72, where I first noticed this problem.

I can try to make a minimal example that shows this problem if it helps.

daniestevez commented 1 year ago

Here is a minimal example.

#!/usr/bin/env python3

from amaranth import *
import amaranth.back.verilog

class TestMemory(Elaboratable):
    def __init__(self, width=16, awidth=8):
        self.w = width
        self.aw = awidth
        self.raddr = Signal(self.aw)
        self.waddr = Signal(self.aw)
        self.rdata = Signal(self.w)
        self.wdata = Signal(self.w)
        self.wren = Signal()

    def elaborate(self, platform):
        m = Module()
        mem = Memory(width=self.w, depth=2**self.aw,
                     attrs={'ram_style': 'block'})
        m.submodules.rdport = rdport = mem.read_port(
                transparent=False)
        m.submodules.wrport = wrport = mem.write_port()
        m.d.comb += [
            rdport.en.eq(1),
            wrport.en.eq(self.wren),
            rdport.addr.eq(self.raddr),
            wrport.addr.eq(self.waddr),
            wrport.data.eq(self.wdata),
            self.rdata.eq(rdport.data),
        ]
        return m

with open('test_memory.v', 'w') as f:
    top = TestMemory()
    f.write(amaranth.back.verilog.convert(
        top, ports=[top.raddr, top.waddr, top.rdata, top.wdata, top.wren]))

The old version of the toolchain generates

/* Generated by Yosys 0.30+1 (git sha1 236e15f3b, clang 10.0.0-4ubuntu1 -fPIC -Os) */

(* \amaranth.hierarchy  = "top" *)
(* top =  1  *)
(* generator = "Amaranth" *)
module top(waddr, rdata, wdata, wren, clk, rst, raddr);
  (* src = "/usr/local/lib/python3.10/dist-packages/amaranth/hdl/ir.py:527" *)
  input clk;
  wire clk;
  (* src = "/hdl/./test_memory.py:20" *)
  wire [7:0] mem_r_addr;
  (* src = "/hdl/./test_memory.py:20" *)
  wire [15:0] mem_r_data;
  (* src = "/hdl/./test_memory.py:20" *)
  wire mem_r_en;
  (* src = "/hdl/./test_memory.py:22" *)
  wire [7:0] mem_w_addr;
  (* src = "/hdl/./test_memory.py:22" *)
  wire [15:0] mem_w_data;
  (* src = "/hdl/./test_memory.py:22" *)
  wire mem_w_en;
  (* src = "/hdl/./test_memory.py:10" *)
  input [7:0] raddr;
  wire [7:0] raddr;
  (* src = "/hdl/./test_memory.py:12" *)
  output [15:0] rdata;
  wire [15:0] rdata;
  (* src = "/usr/local/lib/python3.10/dist-packages/amaranth/hdl/ir.py:527" *)
  input rst;
  wire rst;
  (* src = "/hdl/./test_memory.py:11" *)
  input [7:0] waddr;
  wire [7:0] waddr;
  (* src = "/hdl/./test_memory.py:13" *)
  input [15:0] wdata;
  wire [15:0] wdata;
  (* src = "/hdl/./test_memory.py:14" *)
  input wren;
  wire wren;
  (* ram_style = "block" *)
  reg [15:0] mem [255:0];
  initial begin
    mem[0] = 16'h0000;
    mem[1] = 16'h0000;
    mem[2] = 16'h0000;
    mem[3] = 16'h0000;
    mem[4] = 16'h0000;
    mem[5] = 16'h0000;
    mem[6] = 16'h0000;
    mem[7] = 16'h0000;
    mem[8] = 16'h0000;
    mem[9] = 16'h0000;
    mem[10] = 16'h0000;
    mem[11] = 16'h0000;
    mem[12] = 16'h0000;
    mem[13] = 16'h0000;
    mem[14] = 16'h0000;
    mem[15] = 16'h0000;
    mem[16] = 16'h0000;
    mem[17] = 16'h0000;
    mem[18] = 16'h0000;
    mem[19] = 16'h0000;
    mem[20] = 16'h0000;
    mem[21] = 16'h0000;
    mem[22] = 16'h0000;
    mem[23] = 16'h0000;
    mem[24] = 16'h0000;
    mem[25] = 16'h0000;
    mem[26] = 16'h0000;
    mem[27] = 16'h0000;
    mem[28] = 16'h0000;
    mem[29] = 16'h0000;
    mem[30] = 16'h0000;
    mem[31] = 16'h0000;
    mem[32] = 16'h0000;
    mem[33] = 16'h0000;
    mem[34] = 16'h0000;
    mem[35] = 16'h0000;
    mem[36] = 16'h0000;
    mem[37] = 16'h0000;
    mem[38] = 16'h0000;
    mem[39] = 16'h0000;
    mem[40] = 16'h0000;
    mem[41] = 16'h0000;
    mem[42] = 16'h0000;
    mem[43] = 16'h0000;
    mem[44] = 16'h0000;
    mem[45] = 16'h0000;
    mem[46] = 16'h0000;
    mem[47] = 16'h0000;
    mem[48] = 16'h0000;
    mem[49] = 16'h0000;
    mem[50] = 16'h0000;
    mem[51] = 16'h0000;
    mem[52] = 16'h0000;
    mem[53] = 16'h0000;
    mem[54] = 16'h0000;
    mem[55] = 16'h0000;
    mem[56] = 16'h0000;
    mem[57] = 16'h0000;
    mem[58] = 16'h0000;
    mem[59] = 16'h0000;
    mem[60] = 16'h0000;
    mem[61] = 16'h0000;
    mem[62] = 16'h0000;
    mem[63] = 16'h0000;
    mem[64] = 16'h0000;
    mem[65] = 16'h0000;
    mem[66] = 16'h0000;
    mem[67] = 16'h0000;
    mem[68] = 16'h0000;
    mem[69] = 16'h0000;
    mem[70] = 16'h0000;
    mem[71] = 16'h0000;
    mem[72] = 16'h0000;
    mem[73] = 16'h0000;
    mem[74] = 16'h0000;
    mem[75] = 16'h0000;
    mem[76] = 16'h0000;
    mem[77] = 16'h0000;
    mem[78] = 16'h0000;
    mem[79] = 16'h0000;
    mem[80] = 16'h0000;
    mem[81] = 16'h0000;
    mem[82] = 16'h0000;
    mem[83] = 16'h0000;
    mem[84] = 16'h0000;
    mem[85] = 16'h0000;
    mem[86] = 16'h0000;
    mem[87] = 16'h0000;
    mem[88] = 16'h0000;
    mem[89] = 16'h0000;
    mem[90] = 16'h0000;
    mem[91] = 16'h0000;
    mem[92] = 16'h0000;
    mem[93] = 16'h0000;
    mem[94] = 16'h0000;
    mem[95] = 16'h0000;
    mem[96] = 16'h0000;
    mem[97] = 16'h0000;
    mem[98] = 16'h0000;
    mem[99] = 16'h0000;
    mem[100] = 16'h0000;
    mem[101] = 16'h0000;
    mem[102] = 16'h0000;
    mem[103] = 16'h0000;
    mem[104] = 16'h0000;
    mem[105] = 16'h0000;
    mem[106] = 16'h0000;
    mem[107] = 16'h0000;
    mem[108] = 16'h0000;
    mem[109] = 16'h0000;
    mem[110] = 16'h0000;
    mem[111] = 16'h0000;
    mem[112] = 16'h0000;
    mem[113] = 16'h0000;
    mem[114] = 16'h0000;
    mem[115] = 16'h0000;
    mem[116] = 16'h0000;
    mem[117] = 16'h0000;
    mem[118] = 16'h0000;
    mem[119] = 16'h0000;
    mem[120] = 16'h0000;
    mem[121] = 16'h0000;
    mem[122] = 16'h0000;
    mem[123] = 16'h0000;
    mem[124] = 16'h0000;
    mem[125] = 16'h0000;
    mem[126] = 16'h0000;
    mem[127] = 16'h0000;
    mem[128] = 16'h0000;
    mem[129] = 16'h0000;
    mem[130] = 16'h0000;
    mem[131] = 16'h0000;
    mem[132] = 16'h0000;
    mem[133] = 16'h0000;
    mem[134] = 16'h0000;
    mem[135] = 16'h0000;
    mem[136] = 16'h0000;
    mem[137] = 16'h0000;
    mem[138] = 16'h0000;
    mem[139] = 16'h0000;
    mem[140] = 16'h0000;
    mem[141] = 16'h0000;
    mem[142] = 16'h0000;
    mem[143] = 16'h0000;
    mem[144] = 16'h0000;
    mem[145] = 16'h0000;
    mem[146] = 16'h0000;
    mem[147] = 16'h0000;
    mem[148] = 16'h0000;
    mem[149] = 16'h0000;
    mem[150] = 16'h0000;
    mem[151] = 16'h0000;
    mem[152] = 16'h0000;
    mem[153] = 16'h0000;
    mem[154] = 16'h0000;
    mem[155] = 16'h0000;
    mem[156] = 16'h0000;
    mem[157] = 16'h0000;
    mem[158] = 16'h0000;
    mem[159] = 16'h0000;
    mem[160] = 16'h0000;
    mem[161] = 16'h0000;
    mem[162] = 16'h0000;
    mem[163] = 16'h0000;
    mem[164] = 16'h0000;
    mem[165] = 16'h0000;
    mem[166] = 16'h0000;
    mem[167] = 16'h0000;
    mem[168] = 16'h0000;
    mem[169] = 16'h0000;
    mem[170] = 16'h0000;
    mem[171] = 16'h0000;
    mem[172] = 16'h0000;
    mem[173] = 16'h0000;
    mem[174] = 16'h0000;
    mem[175] = 16'h0000;
    mem[176] = 16'h0000;
    mem[177] = 16'h0000;
    mem[178] = 16'h0000;
    mem[179] = 16'h0000;
    mem[180] = 16'h0000;
    mem[181] = 16'h0000;
    mem[182] = 16'h0000;
    mem[183] = 16'h0000;
    mem[184] = 16'h0000;
    mem[185] = 16'h0000;
    mem[186] = 16'h0000;
    mem[187] = 16'h0000;
    mem[188] = 16'h0000;
    mem[189] = 16'h0000;
    mem[190] = 16'h0000;
    mem[191] = 16'h0000;
    mem[192] = 16'h0000;
    mem[193] = 16'h0000;
    mem[194] = 16'h0000;
    mem[195] = 16'h0000;
    mem[196] = 16'h0000;
    mem[197] = 16'h0000;
    mem[198] = 16'h0000;
    mem[199] = 16'h0000;
    mem[200] = 16'h0000;
    mem[201] = 16'h0000;
    mem[202] = 16'h0000;
    mem[203] = 16'h0000;
    mem[204] = 16'h0000;
    mem[205] = 16'h0000;
    mem[206] = 16'h0000;
    mem[207] = 16'h0000;
    mem[208] = 16'h0000;
    mem[209] = 16'h0000;
    mem[210] = 16'h0000;
    mem[211] = 16'h0000;
    mem[212] = 16'h0000;
    mem[213] = 16'h0000;
    mem[214] = 16'h0000;
    mem[215] = 16'h0000;
    mem[216] = 16'h0000;
    mem[217] = 16'h0000;
    mem[218] = 16'h0000;
    mem[219] = 16'h0000;
    mem[220] = 16'h0000;
    mem[221] = 16'h0000;
    mem[222] = 16'h0000;
    mem[223] = 16'h0000;
    mem[224] = 16'h0000;
    mem[225] = 16'h0000;
    mem[226] = 16'h0000;
    mem[227] = 16'h0000;
    mem[228] = 16'h0000;
    mem[229] = 16'h0000;
    mem[230] = 16'h0000;
    mem[231] = 16'h0000;
    mem[232] = 16'h0000;
    mem[233] = 16'h0000;
    mem[234] = 16'h0000;
    mem[235] = 16'h0000;
    mem[236] = 16'h0000;
    mem[237] = 16'h0000;
    mem[238] = 16'h0000;
    mem[239] = 16'h0000;
    mem[240] = 16'h0000;
    mem[241] = 16'h0000;
    mem[242] = 16'h0000;
    mem[243] = 16'h0000;
    mem[244] = 16'h0000;
    mem[245] = 16'h0000;
    mem[246] = 16'h0000;
    mem[247] = 16'h0000;
    mem[248] = 16'h0000;
    mem[249] = 16'h0000;
    mem[250] = 16'h0000;
    mem[251] = 16'h0000;
    mem[252] = 16'h0000;
    mem[253] = 16'h0000;
    mem[254] = 16'h0000;
    mem[255] = 16'h0000;
  end
  always @(posedge clk) begin
    if (mem_w_en)
      mem[mem_w_addr] <= mem_w_data;
  end
  reg [15:0] _0_;
  always @(posedge clk) begin
    _0_ <= mem[mem_r_addr];
  end
  assign mem_r_data = _0_;
  assign rdata = mem_r_data;
  assign mem_w_data = wdata;
  assign mem_w_addr = waddr;
  assign mem_r_addr = raddr;
  assign mem_w_en = wren;
  assign mem_r_en = 1'h1;
endmodule

The new version of the toolchain generates

/* Generated by Yosys 0.32+76 (git sha1 73cb4977b, clang 10.0.0-4ubuntu1 -fPIC -Os) */

(* \amaranth.hierarchy  = "top" *)
(* top =  1  *)
(* generator = "Amaranth" *)
module top(waddr, rdata, wdata, wren, clk, rst, raddr);
  (* src = "/usr/local/lib/python3.10/dist-packages/amaranth/hdl/ir.py:508" *)
  input clk;
  wire clk;
  (* src = "/hdl/./test_memory.py:20" *)
  wire [7:0] mem_r_addr;
  (* src = "/hdl/./test_memory.py:20" *)
  wire [15:0] mem_r_data;
  (* src = "/hdl/./test_memory.py:20" *)
  wire mem_r_en;
  (* src = "/hdl/./test_memory.py:22" *)
  wire [7:0] mem_w_addr;
  (* src = "/hdl/./test_memory.py:22" *)
  wire [15:0] mem_w_data;
  (* src = "/hdl/./test_memory.py:22" *)
  wire mem_w_en;
  (* src = "/hdl/./test_memory.py:10" *)
  input [7:0] raddr;
  wire [7:0] raddr;
  (* src = "/hdl/./test_memory.py:12" *)
  output [15:0] rdata;
  wire [15:0] rdata;
  (* src = "/usr/local/lib/python3.10/dist-packages/amaranth/hdl/ir.py:508" *)
  input rst;
  wire rst;
  (* src = "/hdl/./test_memory.py:11" *)
  input [7:0] waddr;
  wire [7:0] waddr;
  (* src = "/hdl/./test_memory.py:13" *)
  input [15:0] wdata;
  wire [15:0] wdata;
  (* src = "/hdl/./test_memory.py:14" *)
  input wren;
  wire wren;
  (* ram_style = "block" *)
  reg [15:0] \$rdport  [255:0];
  always @(posedge clk) begin
    if (mem_w_en)
      \$rdport [mem_w_addr] <= mem_w_data;
  end
  reg [15:0] _0_;
  always @(posedge clk) begin
    _0_ <= \$rdport [mem_r_addr];
  end
  initial _0_ = 16'h0000;
  assign mem_r_data = _0_;
  wrport wrport (
  );
  assign rdata = mem_r_data;
  assign mem_w_data = wdata;
  assign mem_w_addr = waddr;
  assign mem_r_addr = raddr;
  assign mem_w_en = wren;
  assign mem_r_en = 1'h1;
endmodule

(* \amaranth.hierarchy  = "top.wrport" *)
(* generator = "Amaranth" *)
module wrport();
  wire \$empty_module_filler ;
endmodule

Looking at the diff, besides irrelevant things such as the old version initializing the memory to zeros and calling it mem and the new version not initializing it and calling it \rdport, the only difference I see is that the new version includes

wrport wrport (
);

which seems to come out of nowhere.

daniestevez commented 1 year ago

The problem appears to be in amaranth rather than Yosys. I'm now running the current amaranth main with the 20230610 Yosys and the empty wrport module is generated.

daniestevez commented 1 year ago

I've bisected the problem to 8c4a15ab92af10d5640be4d15f27f0e8843ec154.

(Sorry for writing a whole bunch of comments as I debug this further).

whitequark commented 1 year ago

@daniestevez Thanks! Feel free to stop debugging, I know what change it was (though you figured it out) and how to address it.

whitequark commented 1 year ago

It's actually a bug I introduced back in 2020.

whitequark commented 1 year ago

besides irrelevant things such as the old version initializing the memory to zeros and [...] and the new version not initializing it [...]

This isn't irrelevant at all! That's actually another bug.