Regression in synthesis for synchronous memory (ice40)

Steps to reproduce the issue

Attached a .zip that includes the input verilog file (stencil.v), the input .pcf for arachne-pnr, and the output .blif from yosys. Also includes a Makefile with the relevant yosys and arachne-pnr command.

stencil.zip

arachne-pnr version

❯ arachne-pnr -v
arachne-pnr 0.1+207+ 0 (git sha1 1e81432, c++ 9.0.0 -O2)

Checkout and install yosys 4f31cb6d

❯ git checkout 61f681162754fa170494e567f1a1a9ae2d17eb69
...
HEAD is now at 61f68116 Yosys 0.7

NOTE: Because abc has moved to github and removed the makefile from the default clone of their repository, I had to make the following patch to the makefile, otherwise it fails when trying to call make clean

❯ git diff
diff --git a/Makefile b/Makefile
index 0a61fe65..d11a0a65 100644
--- a/Makefile
+++ b/Makefile
@@ -393,7 +393,7 @@ ifneq ($(ABCREV),default)
                test $(ABCPULL) -ne 0 || { echo 'REEBE: NOP abg hc gb qngr naq NOPCHYY frg gb 0 va Znxrsvyr!' | tr 'A-Za-z' 'N-ZA-Mn-za-m'; exit 1; }; \
                echo "Pulling ABC from $(ABCURL):"; set -x; \
                test -d abc || hg clone $(ABCURL) abc; \
-               cd abc && $(MAKE) DEP= clean && hg pull && hg update -r $(ABCREV); \
+               cd abc && hg pull && hg update -r $(ABCREV); \
        fi
 endif
        $(Q) rm -f abc/abc-[0-9a-f]*

❯ make -j
❯ sudo make install

run make with files in attached .zip (icepack command running not required, should just show arachne-pnr not failing. Here is the output .blif file: blif.zip

❯ wget https://github.com/YosysHQ/yosys/files/2215703/stencil.zip
❯ unzip stencil.zip
❯ cd stencil
❯ make
yosys -q -p 'synth_ice40 -top main -blif stencil.blif' stencil.v
Warning: Yosys has only limited support for tri-state logic at the moment. (stencil.v:735)
arachne-pnr -q -d 1k -o stencil.txt -p stencil.pcf stencil.blif
icepack stencil.txt stencil.bin

Checkout and install yosys master

❯ cd ..
❯ git checkout Makefile  # revert Makefile patch
❯ rm -r abc  # so it clones the git version
❯ git checkout master
❯ git show
commit 323f6f6f6006eadcaec180f2cc1556f1f3303be3 (HEAD -> master, origin/master, origin/HEAD)
Merge: 1de07eee 68b5d0c3
Author: Clifford Wolf <clifford@clifford.at>
Date:   Fri Jul 20 19:22:59 2018 +0200

    Merge pull request #586 from hzeller/more-sourcepos-logging

    Convert more log_error() to log_file_error() where possible.
❯ make -j
❯ sudo make install

run make with files in attached .zip

wget https://github.com/YosysHQ/yosys/files/2215703/stencil.zip
❯ unzip stencil.zip
❯ cd stencil
❯ make
yosys -q -p 'synth_ice40 -top main -blif stencil.blif' stencil.v
Warning: Yosys has only limited support for tri-state logic at the moment. (stencil.v:735)
arachne-pnr -q -d 1k -o stencil.txt -p stencil.pcf stencil.blif
stencil.blif:1138: fatal error: unknown model `$mem'
make: *** [stencil.bin] Error 1

Here's the blif file that it generates: blif.zip

Expected behavior

An old version of yosys was able to correctly compile the coreir_mem module for the ice40 target to a .blif file that arachne-pnr could place and route.

Actual behavior

The latest version of yosys master produces a $mem cell which arachne-pnr cannot place.

The verilog module of interest is copied below

module coreir_mem #(parameter depth=1, parameter has_init=1, parameter width=1) (
  input clk,
  input [width-1:0] wdata,
  input [$clog2(depth)-1:0] waddr,
  input wen,
  output reg [width-1:0] rdata,
  input [$clog2(depth)-1:0] raddr
);
  reg [width-1:0] data[depth-1:0];
  always @(posedge clk) begin
    if (wen) begin
      data[waddr] <= wdata;
    end
    rdata <= data[raddr];
  end

endmodule //coreir_mem

It seems that when I make a smaller design using the memory, such as

module coreir_mem #(parameter depth=1, parameter has_init=1, parameter width=1) (
  input clk,
  input [width-1:0] wdata,
  input [$clog2(depth)-1:0] waddr,
  input wen,
  output reg [width-1:0] rdata,
  input [$clog2(depth)-1:0] raddr
);
  reg [width-1:0] data[depth-1:0];
  always @(posedge clk) begin
    if (wen) begin
      data[waddr] <= wdata;
    end
    rdata <= data[raddr];
  end

endmodule //coreir_mem

module main (output [4:0] J3, input  CLKIN);
  wire [7:0] mem__rdata;
  coreir_mem #(.depth(16),.has_init(0),.width(8)) mem(
    .clk(CLKIN),
    .raddr(0),
    .rdata(mem__rdata),
    .waddr(0),
    .wdata(mem__rdata),
    .wen(1)
  );
  assign J3 = mem__rdata[4:0];
endmodule

yosys and arachne can compile it. Here is the input verilog and output blif for the above small example (also includes .pcf and makefile) small.zip

As far as I can tell, what is happening here is that the memory map process is failing to correctly identify the write port, thus producing a memory with an asynchronous write port that cannot be mapped in any sensible way.

Clearly the memory is being written in a synchronous way however. Running this script, which is the synthesis process until anything memory-related starts:

read_verilog -lib  +/ice40/cells_sim.v
read_verilog stencil.v
hierarchy -top main
proc
flatten
opt_expr
opt_clean
opt
wreduce
alumacc
share
opt
fsm
opt -fast
write_ilang dump.ilang

Shows the following write port - at this point lacking a clock is OK because memory_dff hasn't been run yet:

  cell $memwr $techmap\inst49.inst0.lbmem_1_0.mem.$memwr$\data$stencil.v:750$74
    parameter \ABITS 4
    parameter \CLK_ENABLE 0
    parameter \CLK_POLARITY 0
    parameter \MEMID "\\inst49.inst0.lbmem_1_0.mem.data"
    parameter \PRIORITY 74
    parameter \WIDTH 8
    connect \ADDR $techmap\inst49.inst0.lbmem_1_0.mem.$memwr$\data$stencil.v:750$68_ADDR
    connect \CLK 1'x
    connect \DATA \inst49.inst0.lb1d_0.reg_1.outReg
    connect \EN 8'11111111
  end

Then running memory_dff, we get:

16. Executing MEMORY_DFF pass (merging $dff cells to $memrd and $memwr).
Checking cell `$techmap\inst49.inst0.lbmem_1_0.mem.$memwr$\data$stencil.v:750$74' in module `\main': no (compatible) $dff for data input found.
Checking cell `$techmap\inst49.inst0.lbmem_1_0.mem.$memrd$\data$stencil.v:752$73' in module `\main': merged data $dff to cell.

This shows there is a problem with the data input. Looking at the data port, in the ilang dump, we see that although the net connect to the data input is correctly driven by a $dff, it also fans directly out to a number of LUT inputs which means that it can't be folded into the memory write port. I suspect this is an overly eager optimisation pass doing this.

I think this could be solved in two ways:

Preventing this optimisation happening when a net connects to a $memwr input
Duplicating the $dff in $memory_dff Not sure what you think @cliffordwolf ?

YosysHQ / yosys