Symbiflow-classroom:Reset of BRAM Output Register

[nelsobe]

SYMBIFLOW-CLASSROOM-PROJECT

Below is a snippet of code from a riscv design used in our junior level course.

As can be seen, the reading from the BRAM includes a reset condition as well as a conditional read.

When processed by the standard Yosys front end, the inst_memory is not mapped to a BRAM but it seems is mapped to FF's, resulting in a non-functional design (curiously, this seems to cause other sections of the design to simply be optimized away). Removing the conditional read clauses in the code does result in mapping it to a BRAM.

I am guessing this is a BRAM mode not yet supported by Yosys?

// Instruction memory (use property to make sure it is mapped to a BRAM)
(* rom_style = "block" *) reg [31:0] inst_memory [0:INSTRUCTION_WORDS-1];
always_ff @(posedge clk)
    begin
        if (rst)
            instruction <= 0;
        else if(iMemRead == 1 && valid_upper_text_address)
            instruction <= inst_memory[PC[INSTRUCTION_ADDR_BITS-1:2]];
    end

[mkurc-ant]

It seems that Yosys expects the attribute to be ram_style instead of rom_style. See: https://github.com/YosysHQ/yosys/blob/322ab1cd54f4cddba4e9408887ed822c541185f9/techlibs/xilinx/xc7_xcu_brams.txt#L92

[nelsobe]

@mkurc-ant I have attached the synth_log as well as the original .sv file (part of a bigger project).

It looks like yosys is not inferring a bram for the inst_memory.

As I said in the meeting, our class is stalled due to this bug. If a fix could be found quickly (within a few days) we can get the rest of the clas's riscv processor variations tested through the toolchain before the semester ends in a couple of weeks. If not, we will miss out until next year. So, if there is a way to address it quickly it could help a lot with the testing project.

multicycle_iosystem.zip

[mkurc-ant]

@nelsobe Thank you, I'll take a look on that today.

[mkurc-ant]

@nelsobe Let me share my findings:

First of all it looks like you are using an outdated Yosys version (Yosys 0.9+4270 git sha1 539d4ee9) which seems to cause problem in BRAM inference in some cases. I've tried the upstream one that is Yosys 0.15+44 git sha1 3bf107024. With the upstream version there were no issues with inferring BRAM for inst_memory whatsoever.

Unfortunately the memory inference does not work with the version currently supposed to be used in symbiflow / f4pga which is Yosys 0.13+3 git sha1 61324cf55 as well. I'll create a PR which bumps its version.

Another thing is the attribute. As I mentioned before Yosys expects the ram_style attribute while Vivado supports both ram_style and rom_style (those behave differently in some cases but here it shouldn't matter). Setting ram_style = "block" effectively forces Yosys to use a BRAM but even without it the upstream Yosys version decided that use of a BRAM is feasible. So it got inferred anyways.

So my suggestion is to try the upstream Yosys (there should be a conda package for the latest version available). I'll create a PR which bumps the version in the f4pga-arch-defs repository. As for the attribute we can think of adding support for rom_style to upstream Yosys.

[mithro]

@mkurc-ant - Why is our version of Yosys so old? Isn't it suppose to be auto-updating?

[mkurc-ant]

@mithro I need to confirm that but it may be that the dependabot creates a PR for each new Yosys package and it just can't keep up with the pace they are added.

[nelsobe]

@mkurc-ant @tmichalak
Thanks for the follow ups we are seeing on some of these issues. We are slowly debugging and doing workarounds and now have a handful of our student labs now operational. Bug/issues have been filed on everything we have file thus far.

But, as @mithro pointed out in our meeting last week, in our role as users of the tools (instead of developers) my students (and me) are not privy to the inner details of what pieces/plugins/tarFiles go together to make a full toolchain. As such, our model is to simply reinstall when we are told something has been fixed. To help so we can more intelligently communicate with you on these issues could you quickly answer a couple of questions:

1. Without doing a full reinstall, what is a good way to tell if the tools we are using are the latest and are using the latest versions of yosys, surelog, other plugins, ...?

2. We sense that when a plug-in (like surelog) is updated to fix a bug, that doesn't necessarily mean we will immediately get access to it just by reinstalling f4pga-examples - is this correct? What else has to happen before we get access just by reinstalling f4pga-examples? How can we tell when that has happened?

3. Is there an alternative to a full reinstall when a plugin (like surelog) is updated?

4. Last week there was talk of modifying things so that the surelog plug is just included by default. Is that complete yet? How will we know when it is?

Thanks much!

[nelsobe]

@mkurc-ant @tmichalak @mithro
Any news on item 4 in the comment above? We would like to test the latest Surelog plugin with f4pga-examples and are waiting for it to be supported by the synthesis scripts. Thanks.

[nelsobe]

@mkurc-ant @tmichalak Have confirmed the error has to do with the reset clause, NOT the conditional read clause. Assume, but don't know for sure, that this is a synthesis problem and not a parsing problem and therefore the surelog plugin will not solve this.

[nelsobe]

@mkurc-ant @tmichalak Any word on this one? As mentioned above, I have confirmed the error has to do with the reset clause in the code.

[mkurc-ant]

@nelsobe We have checked the issue against Yosys and it looks like the problem is in the memory inference mechanism in Yosys itself.

In your design you are using the specific Xilinx feature where the output register of a BRAM can be externally reset to all-zeros. However, memory inference in Yosys is generic, it can infer a BRAM with output register but it is completely unaware of the possibility of resetting that register.

This shortcoming results in Yosys not inferring a BRAM in your case because the register following the memory cell is incompatible (has a reset).