minerva-cpu / minerva

A 32-bit RISC-V soft processor
Other
305 stars 32 forks source link

Unable to simulate with Verilator: Circular logic error #5

Closed enjoy-digital closed 5 years ago

enjoy-digital commented 5 years ago

When elaborating Minerva with upstream sources and tools (Minerva, nMigen, Yosys), verilator does not seem able to compile the elaborated verilog:

%Error: minerva.v:10300: Wire inputs its own output, creating circular logic (wire x=x)
%Error: minerva.v:10226: Wire inputs its own output, creating circular logic (wire x=x)
%Error: minerva.v:9597: Wire inputs its own output, creating circular logic (wire x=x)

The CPU has been generated with:

python3 cli.py --reset-addr=0 --with-muldiv

Assuming a RISC-V toolchain and Verilator are installed, the error can be reproduced with:

wget https://raw.githubusercontent.com/enjoy-digital/litex/master/litex_setup.py
chmod +x litex_setup.py
sudo ./litex_setup.py init install
lxsim --cpu-type=minerva

Here is the elaborated CPU: minerva.zip

whitequark commented 5 years ago

This is an nMigen bug. The problematic lines all are: assign \$verilog_initial_trigger = \$verilog_initial_trigger;, which is a true positive from Verilator's POV. The reason for \$verilog_initial_trigger's existence is that a statement like always @* x = 1; in Verilog will never execute because its implicit event expression list is empty. However, with current Yosys master (specifically, anything that has the proc_prune pass), such pathological statements will never be emitted. (Statements like always @* if(y) x = 1; will be, but these are fine, since a conditional does read something.)

I will fix this as it is quite easy.

whitequark commented 5 years ago

Let me know if this nMigen patch works for you:

diff --git a/nmigen/back/verilog.py b/nmigen/back/verilog.py
index 8faf58a..eb98a34 100644
--- a/nmigen/back/verilog.py
+++ b/nmigen/back/verilog.py
@@ -39,6 +39,7 @@ def _convert_rtlil_text(rtlil_text, *, strip_internal_attrs=False, write_verilog
 read_ilang <<rtlil
 {}
 rtlil
+{prune}delete w:$verilog_initial_trigger
 {prune}proc_prune
 proc_init
 proc_arst
enjoy-digital commented 5 years ago

Thanks for the quick answer/patch, simulation is working correctly with it :+1:

whitequark commented 5 years ago

https://github.com/m-labs/nmigen/commit/4d6ad28f5966f1f7c94a8f83ef50f07d7e62123c