Removing a verilog comment changes behaviour.

[joseluisquiroga]

Steps to reproduce the issue

I have created an small project to reproduce: https://github.com/messaging-cells/bug_synth_03

All info to reproduce the bug in the README file.

File with the comment to be removed in order to reproduce bug: https://github.com/messaging-cells/bug_synth_03/blob/master/rtl/io_bug_03.v

Expected behavior

For both cases to have the same behaviour.

[dh73]

Hi @joseluisquiroga ,

Have you tried simulating first your design before synthesize it?.

By looking quickly at your repo, I do see some fundamental (and syntactical errors) such as this one: Port definitions in line 35 of the file you mention, expands into a wrong port definiton. The last port is sepparated by a comma, which is not what Verilog expects.

For another example, take a look at tree_nand.v, there is another obvious problem here. Also, when using default_nettype to none, you must explicitly declare port types in all files, there are still some undeclared.

I suggest you to simulate (or use verilator to lint your design) first, and fix all the reported problems before going to Yosys. If the error persist after that (I really doubt it), it will be much more easy to debug it.

[joseluisquiroga]

It is not my intention to degrade the hard and difficult work that the people working in this project are doing. The bug is just a fact.

It seems to me that the rules in

https://github.com/YosysHQ/yosys/blob/1bf2bdf05bd78a08f932780d99144b2d56e2943f/frontends/verilog/verilog_parser.y#L387

https://github.com/YosysHQ/yosys/blob/1bf2bdf05bd78a08f932780d99144b2d56e2943f/frontends/verilog/verilog_parser.y#L390

Are doing their job just fine. And the parser does not show an error at parsing time for that extra comma because the rules allow an empty parameter. So NO. That is not it.

It seems to me that the bug has to do with some kind of optimization during one of the filters.

[boqwxp]

Can you post the write_ilang output for both versions? And is there any way you can reduce the test case to something that doesn't require the physical board and manual verification?

[joseluisquiroga]

Thank you both for caring. Sorry to respond this late. I try not too look at any screens from Friday 6pm to Saturday 6pm.

I updated the builds in the project to have the ilang files (nice option). https://github.com/messaging-cells/bug_synth_03/blob/master/MY_BUILD_WITH_THE_COMMENT/bug_03.ilang https://github.com/messaging-cells/bug_synth_03/blob/master/MY_BUILD_WITHOUT_THE_COMMENT/bug_03.ilang

I think it has to do with array optimizations. Just a hunch. And that this bug is related to my previous bug: https://github.com/YosysHQ/yosys/issues/1760 which was as weird as this one but disappeared while reducing it to its minimal expression (deleting redundant commands in the tcl script).

I think that in order to reduce the test to something virtual there would have to be a way to simulate the json file generated by yosys. I will search for software and see if I can. Maybe ngspice but I have never used or done any thing like it. As you may have both guess it I am a newbie at hardware design.

Thanks again.

P.S. I think I linked the wrong rules in my previous comment. Ups.

[dh73]

It is not my intention to degrade the hard and difficult work that the people working in this project are doing. The bug is just a fact.

It seems to me that the rules in

https://github.com/YosysHQ/yosys/blob/1bf2bdf05bd78a08f932780d99144b2d56e2943f/frontends/verilog/verilog_parser.y#L387

https://github.com/YosysHQ/yosys/blob/1bf2bdf05bd78a08f932780d99144b2d56e2943f/frontends/verilog/verilog_parser.y#L390

Are doing their job just fine. And the parser does not show an error at parsing time for that extra comma because the rules allow an empty parameter. So NO. That is not it.

It seems to me that the bug has to do with some kind of optimization during one of the filters.

I apologize for not be able to explain myself at the beginning, please allow me to try again. What I am saying is more on the sense of what Alberto is saying. I looked at the source code once, very quick, and I saw some syntax errors. Despite of them, Yosys is not reporting any, but fixing some tokens with the Verilog parser.

For instance, your problem as is, removing the comments around line 90 of io_bug_03.v, shows an equivalence problem around the logic that rgo0_dat is driving. That very logic is generated with the macro NS_FIFO_TRY_SET_OUT, which does not seems to expand correctly:

---

File /calc_redun.v, even if Yosys is accepting this input, it is something that definetely is not allowed:

output wire [RSZ-1:0] redun,
);

---

The same problem exists in more files.

File tree_nand.v, a nand is described as ~(terma & termb), here looks incorrect:

assign out_nand = out_nand_low ~& out_nand_high;

But Yosys accepts the token and assigns it correctly:

Dumping Verilog AST after simplification:
    (* dynports = 1 *)
    module tree_nand(in_nand, out_nand);
      /** AST_PARAMETER **/
      input [31:0] in_nand;
      output out_nand;
      wire out_nand_low;
      wire out_nand_high;
      /** AST_GENIF **/
      /** AST_CELL **/
      /** AST_CELL **/
      assign out_nand = ~((out_nand_low)&(out_nand_high));
      /** AST_GENBLOCK **/
      /** AST_GENIF **/
    endmodule
--- END OF AST DUMP ---

And there are more problems like these I described.

---

Now, if you try to synthesize your design, using -noflatten, it seems to show only an equivalence problem in the logic that drives LED3, which for me says that the issue is introduced for all these macros that does not expand correctly, and syntax errors that the tool is not reporting. Because, by flattening the design, the tool can do optimisations that affects the whole logic, and not just by modules (scopes).

So my initial comment was more in the sense that, the problem here could be that the Yosys Verilog parser is accepting this HDL, even if its syntactical wrong, but is something that is known (see point 5), and maybe not a wrong optimisation from a valid HDL code, but to really know that, you should make sure your RTL is healthy, so if a problem really exists, one can remove all unnecessary parts, reduce the problem to a small test case to analyze it faster.

What I would do here, is to start fixing all the syntax errors, make sure the macros expand correctly (or remove them until I know everything is working fine), then try to lint the design with Verilator and/or generate a small testbench to check that my logic is behaving as I want. Then, synthesize it and check if the problem is still present. Because right now there are too many parts moving and its hard to track the real issue.

[joseluisquiroga]

Thanks for explaining. I will give my syntax some "icarus" to get it "healthy" and see what happens.

[joseluisquiroga]

This bug is a fact. The syntax is now iverilog -Wall -g2005 complaint. I have included the expanded code by iverilog (option -E) in the builds. Full code in:

https://github.com/messaging-cells/bug_synth_03

My code only fails WITH the comment and only for this particular setting of clocks for instances gt_BUG_t5 and io_BUG_t5 in file bug_03.v.

WITHOUT the comment I get the code working just as the logic of the written code directs it to do. So I am pretty happy with the logic.

In any case (even if it did not behave as I want it to at all), the expected behavior for the code is to be the same with and without the comment. That is expected of any language.

I find no use in trying to simulate it in icarus verilog or verilator because, as far as I can possibly understand, the problem would not be in the code generated by either of these. It is in the netlist generated by yosys in the json file. So it is like looking for my keys where there is light, not where I lost them.

I still think is an array optimization problem. That is where I would look if I were to find the bug.

Reducing the code more is VERY DIFFICULT for the reasons explained in the README file. So I think it would help a lot the team of yosys if my suggestions in the README are taken for future versions of yosys. Because people like me (newbie) would be able to reduce the code to the simplest expression of the bug.

[daveshah1]

Using this testbench:

module testbench;

reg clk;
always #5 clk = (clk === 1'b0);

wire [3:0] led;

initial begin
        $dumpfile("testbench.vcd");
        $dumpvars(0, testbench);

        repeat (6) begin
            repeat (5000) @(posedge clk);
            $display("+5000 cycles LED=%b", led);
        end
        $finish;
end
test_top top_i(.i_clk(clk), .o_LED_1(led[0]), .o_LED_2(led[1]), .o_LED_3(led[2]), .o_LED_4(led[3]));

endmodule

Generating Verilog from JSON using yosys -p "write_verilog -norename bug_03_syn.v" bug_03.json

and simulating using

iverilog -o bug03 tb.v MY_BUILD_WITHOUT_THE_COMMENT/bug_03_syn.v `yosys-config --datdir`/ice40/cells_sim.v && ./bug03

neither of the two JSON files nor the original RTL turn any of the LEDs on.

While it is remotely possible that there is a nextpnr bug going on, you have several logic-generated clocks and some clock domain crossings which are often the cause of unexpected behaviour (metastability, glitches, etc) that varies depending on nextpnr's "chaotic" placement, which can be peturbed significantly with small input changes. While in an ideal world things would be less chaotic, this is an unfortunate feature of most FPGA toolchains to some greater or lesser extent.

As far as I can tell, there is no Yosys bug here.

UPDATE: post place and route bitstream simulation using icebox_vlog works fine too (no LEDs on) so I'm pretty sure this is a simply incorrect clock crossing or generation.

[joseluisquiroga]

That is the same behavior I got for WITHOUT the comment. It was for the case WITH the comment that the leds turned ON. But I followed your steps for the case WITH the comment and no leds either turned ON in the simulation.

So thanks. I learned something today.

[dh73]

Thank you very much @daveshah1 .

If the issue has reached a satisfactory conclusion, may I suggest closing it?. That will help to maintain things organized and/or using the case as reference in future searches. Thanks!

[joseluisquiroga]

I am sorry for this late comment about this issue. For any given language If nothing semantically changes in the input nothing semantically should change in the output.

In this case the only way to be sure about this is for both netlists (with and without the comment) to be:

• The exact same netlist.
• Semantically equivalent netlist (basically a sat problem)

I do not know how to use your tool to do that, but having a simulation to have the same output is not the way to do either of these.

Thanks for your tool, your hard work and your interest in the issues of this newbie.

[dh73]

@joseluisquiroga , It is a valid concern, an a very good opportunity to learn more about synthesis and place-and-route tools and algorithms.

First of all, you did solved all of your syntactical problems, but you still had semantical ones:

• rgo0_busy and rgo0_added_hd are declared as a variable, but used in blocking context, mixing assignments in process (always) block . This can lead to race conditions and other problems.

After fixing both of them, you can run synthesis with and without removing your comments. I did not dumped a json, nor an ilang, but a verilog file. I ran it twice, one for each case. Then, you can compare both netlist with the following script (I called it equiv.ys:

read_verilog cells_sim.v
design -save read

# with comments
read_verilog golden.v
hierarchy -top test_top
prep -flatten -top test_top
splitnets -ports
design -stash gold

# without comments
read_verilog revised.v cells_sim.v
hierarchy -top test_top_gate
prep -flatten -top test_top_gate
design -stash gate

design -import gold -as gold
design -import gate -as gate

# performing eq check
miter -equiv -flatten -make_assert -make_outputs gold gate miter
sat -verify -prove-asserts -seq 10 -ignore_unknown_cells -show-ports miter

When you execute it (yosys -s equiv.ys), you will see this message, which states that both netlist are equivalent.

Solving problem with 124485 variables and 328540 clauses..
SAT proof finished - no model found: SUCCESS!

                  /$$$$$$      /$$$$$$$$     /$$$$$$$    
                 /$$__  $$    | $$_____/    | $$__  $$   
                | $$  \ $$    | $$          | $$  \ $$   
                | $$  | $$    | $$$$$       | $$  | $$   
                | $$  | $$    | $$__/       | $$  | $$   
                | $$/$$ $$    | $$          | $$  | $$   
                |  $$$$$$/ /$$| $$$$$$$$ /$$| $$$$$$$//$$
                 \____ $$$|__/|________/|__/|_______/|__/
                       \__/                              

Warnings: 16 unique messages, 36 total

I hope this example helps. You can find more information on those commands in the Yosys documentation.

[joseluisquiroga]

Thanks a lot to all of you for all your help. And special thanks to Diego.