icebram incompatible with new memory_libmap

[bjoto]

Version

Yosys 0.21+1 (git sha1 b7bf68501, gcc 11.2.0-19ubuntu1 -fPIC -Os)

Reproduction Steps

I'm getting some strange behavior with yosys >0.17+16, and I think d7dc2313b915 ("ice40: Use memory_libmap pass.") might be the issue.

To reproduce, clone icestorm (https://github.com/YosysHQ/icestorm.git), and build icebram.

cd icebram
make
while true; do bash ./rundemo.sh |& grep 'Found and replaced '; done

Expected Behavior

Found and replaced string should never be zero. All yosys <= 0.17+16 outputs e.g.:

Found and replaced 1 instances of the memory.
Found and replaced 1 instances of the memory.
Found and replaced 2 instances of the memory.
Found and replaced 3 instances of the memory.
Found and replaced 3 instances of the memory.
Found and replaced 2 instances of the memory.
Found and replaced 2 instances of the memory.
Found and replaced 1 instances of the memory.
Found and replaced 1 instances of the memory.
Found and replaced 1 instances of the memory.
Found and replaced 3 instances of the memory.

Actual Behavior

icebram should be able to find a "ram_data" section to replace, but almost always, it fails to do so:

$ while true; do bash ./rundemo.sh |& grep 'Found and replaced '; done
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 1 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 4 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 3 instances of the memory.
Found and replaced 0 instances of the memory.
Found and replaced 0 instances of the memory.
^C

[chrbayer]

The structure of the .ram_data slices seem to be the same but for the same data it seems to be differently encoded so it can't be matched to the from data. Should a issue be opened for icestorm? Without using icebram the result seems to be Ok.

[nakengelhardt]

I've transferred the issue; I'll see if I can find someone more familiar with icebram to take a look.

[lethalbit]

So far I've found that it looks like it is memory geometry dependent, still working on narrowing it down. but for instance, the example with the following geometry will fail:

• bram_width: 12
• bram_depth: 1024
• num_prorts: 1

while the following geometry will always succeed:

• bram_width: 12
• bram_depth: 1280
• num_prorts: 2

The reason the examples seem to fail randomly is because of how the demo is generated, by picking a random width, depth, and port count within a range.

Currently looking into see if this is a synthesis issue or an icebram issue

[chrbayer]

I'm using latest versions of yosys and nextpnr but icebram is not able to find the blocks to replace in the asc file... If useful I can try to provide more data about the geometry I use. I'm not sure that it is a synthesis error, because if I provide correct hex file during synthesis, the design is working...

[lethalbit]

Yeah I suspect it's an icebram issue, looking into seeing what could possibly cause it at the moment.

[niels-moller]

Thanks for filing this issue. I suspect it's the reason icebram doesn't work for me. I'm using Yosys 0.22+57 (git sha1 518194fac, gcc 10.2.1-6 -Og -fPIC), and my design includes a RAM of 1024 words, 64-bits wide.

As far as I understand, this is synthesized as 4x4 SB_RAM40_4K, each configured as 256 words of 16 bits. So both address space and data width are split in four pieces, and there are many ways that could be done (e.g., words could be split in 16-bit chunks, or interleaved, any two bits of the address could be used as a bank select).

I wonder if there's any possibility to have yosys propagate some metadata from the verilog (in my case) source file to the .ram_data lines in the generated .asc file, with an id to the register array in the sources, and how bits are mapped from the source array to this BRAM block? Matching the random initialization data appears a bit brittle.

Sorry, forget to say in which way it doesn't work. I get the icebram messages

  Padding to_hexfile from 1 words to 1024
  Loaded pattern for 64 bits wide and 1024 words deep memory.
  Extracted 256 bit slices from from/to hexfile data.
  Found 28 initialized bram cells in asc file.
  Found and replaced 0 instances of the memory.

where I would expect it to find and replace 1 instance. (There's no interesting initialization data for the 12 additional bram cells, which are for other purposes).

[tomverbeure]

Eric just had a pretty long Yosys bisecting exercise to figure out why his existing design failed after I tried to recompile his example code. (See https://github.com/ICE-V-Wireless/ICE-V-Wireless/commit/38e0798b7f3acd68918e2c34cd632a507d861276.)

If it won't be fixed anytime soon, would it make sense to make icebram print a warning of some sort that there's a known issue?

[niels-moller]

If it won't be fixed anytime soon, would it make sense to make icebram print a warning of some sort that there's a known issue?

To me, it would make sense for icebram to treat "Found and replaced 0 instances of the memory." as a fatal error .

[tomverbeure]

What about changing icebram so that it returns an error code so that most existing Makefiles will abort? I've created a PR for that.

[smunaut]

I rewrote icebram : See this branch in my tree https://github.com/smunaut/icestorm/tree/icebram Testing would be appreciated to see if there are cases where it fails.

[emeb]

This has worked for my failing case.

[dirkenstein]

Works for me, too. At least, does not produce errors. Not tested the actual code yet.

[chrbayer]

For me it works, too. No errors are produced and the design is working, too. So thank you very much!