Icarus Verilog can trigger fatal Multiple assignments to port issue due to simulated xs

[nathanielnrn]

Elaborating on some issues mentioned here.
Haven't had time to create a minimal reproducing example yet, will hopefully be able to work on that after Friday. For now the full steps and code generating the error is listed.

The tl;dr is that simulating verilog via cocotb, using Icarus as our simulator, can sometimes (when setting WAVES=1) creates the following error:

FATAL: /scratch/nrn25/data/calyx-fpga-new/calyx/fpgas/axi-calyx-py/cocotb/../outputs/curr-axi.v:2263: Multiple assignment to port `fsm0.in'. 

The relevant lines in the generated verilog are here.

The steps to reproduce should be as followed

1. Install cocotb

pip install cocotb

2. As well as cocotbext-axi

 pip install cocotbext-axi

(Technically, this step is not needed, the verilog in the branch should suffice but is here for completeness.)
3. Then navigate to calyx/fpgas/axi-calyx-py/ and run fud e axi-reads-calyx.futil --from calyx --to verilog -o outputs/axi-reads.v.
This should create a file identical to the one in the branch

4. Navigate to calyx/fpgas/axi-calyx-py/cocotb/ and run make (relevant Makefile). This executes the cocotb python test defined here. This should terminate normally after simulating for 1020 ns.

5. In the same directory run make WAVES=1. This asks Icarus to create an .fst file. This will give the error

FATAL: /scratch/nrn25/data/calyx-fpga-new/calyx/fpgas/axi-calyx-py/cocotb/../outputs/curr-axi.v:2263: Multiple assignment to port `fsm0.in'. 

Further invocations of make also seem to throw this concurrent assignment error.

To be able to simulate again (i.e going back to step 4) one can run make clean followed by make

[nathanielnrn]

cc @rachitnigam @paili0628 @calebmkim

[rachitnigam]

Okay, I'm going to write down my debugging process. There are a couple of things that stand out as possibly causing problems.

First, I followed the reproduction instructions but couldn't get make to succeed. The simulation fails for me:

rachitnigam ~/git/calyx/fpgas/axi-calyx-py/cocotb (axi-calyx-gen !?$)
% fud e ../axi-reads-calyx.futil --from calyx --to verilog -o ../outputs/axi-reads.v && make                            
rm -f results.xml
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f Makefile results.xml
/opt/homebrew/bin/iverilog -o sim_build/axi-reads/sim.vvp -D COCOTB_SIM=1 -s main -f sim_build/axi-reads/cmds.f -g2012   /Users/rachitnigam/git/calyx/fpgas/axi-calyx-py/cocotb/../outputs/axi-reads.v
/Users/rachitnigam/git/calyx/fpgas/axi-calyx-py/cocotb/../outputs/axi-reads.v:3252: warning: System task ($fatal) cannot be synthesized in an always_comb process.
...
/Users/rachitnigam/git/calyx/fpgas/axi-calyx-py/cocotb/../outputs/axi-reads.v:2783: warning: System task ($fatal) cannot be synthesized in an always_comb process.
rm -f results.xml
MODULE=axi-read-tests TESTCASE= TOPLEVEL=main TOPLEVEL_LANG=verilog \
         /opt/homebrew/bin/vvp -M /Users/rachitnigam/.pyenv/versions/3.10.10/lib/python3.10/site-packages/cocotb/libs -m libcocotbvpi_icarus   sim_build/axi-reads/sim.vvp 
     -.--ns INFO     gpi                                ..mbed/gpi_embed.cpp:78   in set_program_name_in_venv        Did not detect Python virtual environment. Using system-wide Python interpreter
     -.--ns INFO     gpi                                ../gpi/GpiCommon.cpp:101  in gpi_print_registered_impl       VPI registered
     0.00ns INFO     cocotb                             Running on Icarus Verilog version 12.0 (stable)
     ...
     0.00ns INFO     cocotb.main.m                        rvalid width: 1 bits
     0.00ns INFO     cocotb.main.m                      Reset de-asserted
memmap read: xyz
RAM00000000: 78 79 7a 00                                      xyz.
vec1_data: main.vec1_data
  1020.00ns INFO     cocotb.regression                  read_channels_tests failed
                                                        Traceback (most recent call last):
                                                          File "/Users/rachitnigam/git/calyx/fpgas/axi-calyx-py/cocotb/axi-read-tests.py", line 36, in read_channels_tests
                                                            assert main.vec1_data.mem[0] == b"x", "Axi channel failed"
...
                                                          File "/Users/rachitnigam/.pyenv/versions/3.10.10/lib/python3.10/site-packages/cocotb/binary.py", line 61, in resolve_error
                                                            raise ValueError(
                                                        ValueError: Unresolvable bit in binary string: 'x'
...

Not sure why this is happening.

Using synth-verilog Stage

There are bunch of warnings about how $fatal cannot be synthesized. This is because the verification checks generate $fatal to generate errors. We can try to see if targeting the synth-verilog stage in fud, which does not generate these checks, does any better.

% fud e ../axi-reads-calyx.futil --from calyx --to synth-verilog -o ../outputs/axi-reads.v && make 
...
                                                            raise ValueError(
                                                        ValueError: Unresolvable bit in binary string: 'x'

Same error but at least the warnings are gone.

Disabling Optimizations

Maybe one of the optimizations is the problem. Let's disable Calyx optimizations completely:

% fud e ../axi-reads-calyx.futil --from calyx --to synth-verilog -o ../outputs/axi-reads.v -s calyx.flags ' -p no-opt' && make
[fud] ERROR: `/Users/rachitnigam/git/calyx/target/debug/calyx -l /Users/rachitnigam/git/calyx -b verilog --synthesis -p external --disable-verify  -p no-opt' failed:
=====STDERR=====
Error: Found combinational cycle:
block_transfer_done.in = block_transfer_done_and.out;
block_transfer_go.in = !block_transfer_done.out & fsm0.out == 4'd3 & tdcc_go.out ? 1'd1;
block_transfer_done_and.left = block_transfer_go.out ? ARREADY;
block_transfer_done_and.right = block_transfer_go.out ? is_arvalid.out;

Interesting! The compiler detects an obvious combinational loop after lowering the program. This comes from this group:

      group block_transfer{
          block_transfer_done_and.left = ARREADY;
          block_transfer_done_and.right = is_arvalid.out;
          block_transfer[done] = block_transfer_done_and.out;
      }

The problem is that the done condition is defined using a combinational component (a std_and component). #1303 talks about this problem.

Concluding this comment while I try out fixes.

[rachitnigam]

Added the following patch (ignore the whitespace changes) which fixes the combinational loop. The high-level idea is putting the output from the std_and into a register and using the register's output.

Because we're using the register, we need to reset the value in the register every time we loop which is why we add another group.

@nathanielnrn can you take a shot at using this?

diff --git a/fpgas/axi-calyx-py/axi-reads-calyx.futil b/fpgas/axi-calyx-py/axi-reads-calyx.futil
index 57f382ce..71af30c6 100644
--- a/fpgas/axi-calyx-py/axi-reads-calyx.futil
+++ b/fpgas/axi-calyx-py/axi-reads-calyx.futil
@@ -31,12 +31,12 @@ component m_arread_channel(
   ARSIZE: 3,
 
   ARLEN : 8, //in AXI4 this is 8 bits, 1-256 transfers in requested transaction.
-  
+
   // 00 for fixed, 01 for incrementing, 2 for wrap, needs to be incr)
   ARBURST : 2) {
   cells{
       is_arvalid = std_reg(1);
-      //todo(nathanielnrn): we get this from `s_axi_control`. Need to check 
+      //todo(nathanielnrn): we get this from `s_axi_control`. Need to check
       //how we known actual address
       //should probably live in `s_axi_control` but for now will live here
       ref base_addr = std_reg(64);
@@ -51,7 +51,7 @@ component m_arread_channel(
       txn_adder = std_add(32);
 
       block_transfer_done_and = std_and(1);
-
+      bt_reg = std_reg(1);
 
   }
 
@@ -64,20 +64,28 @@ component m_arread_channel(
           is_arvalid.write_en = 1'b1;
           assert_val[done] = is_arvalid.done;
       }
-      
+
       group deassert_val {
           is_arvalid.in = 1'b0;
           is_arvalid.write_en = 1'b1;
           deassert_val[done] = is_arvalid.done;
       }
 
+      group reset_bt {
+        bt_reg.in = 1'b0;
+        bt_reg.write_en = 1'b1;
+        reset_bt[done] = bt_reg.done;
+      }
+
       group block_transfer{
           block_transfer_done_and.left = ARREADY;
           block_transfer_done_and.right = is_arvalid.out;
-          block_transfer[done] = block_transfer_done_and.out;
+          bt_reg.in = block_transfer_done_and.out;
+          bt_reg.write_en = 1'b1;
+          block_transfer[done] = bt_reg.out;
       }
 
-      group do_ar_transfer { 
+      group do_ar_transfer {
           ARADDR = base_addr.out;
           ARSIZE = 3'b110; //see link above, needs to match data width to host.
           //For now this can be taken from .yxi, as size of mem.
@@ -119,6 +127,7 @@ component m_arread_channel(
           txn_init;
           while perform_reads.out with check_reads_done{
               seq{
+                  reset_bt;
                   assert_val;
                   block_transfer;
                   deassert_val;
@@ -147,7 +156,7 @@ component m_read_channel(
       ref data_received = seq_mem_d1(32, 16, 64);
       is_rdy = std_reg(1);
       ref curr_addr = std_reg(64); //TODO(nathanielnrn): not used in axi, only internal
-      
+
       n_RLAST = std_not(1);
 
 
@@ -159,6 +168,7 @@ component m_read_channel(
 
 
       block_transfer_done_and = std_and(1);
+      bt_reg = std_reg(1);
 
   }
   wires{
@@ -185,10 +195,18 @@ component m_read_channel(
           deassert_rdy[done] = is_rdy.done;
       }
 
+      group init_bt {
+        bt_reg.in = 1'b0;
+        bt_reg.write_en = 1'b1;
+        init_bt[done] = bt_reg.done;
+      }
+
       group block_transfer {
         block_transfer_done_and.left = is_rdy.out;
         block_transfer_done_and.right = RVALID;
-        block_transfer[done] = block_transfer_done_and.out;
+        bt_reg.in = block_transfer_done_and.out;
+        bt_reg.write_en = 1'b1;
+        block_transfer[done] = bt_reg.out;
         //block_transfer[done] = !(is_rdy.out & RVALID) ? 1'b0; //good to be explicit and not rely on undefined behavior
       }
 
@@ -205,6 +223,7 @@ component m_read_channel(
       //XXX(nathanielnrn): is this it? Do we need any other conditions?
       while n_RLAST.out with not_rlast{
           seq{
+            init_bt;
               assert_rdy;
               block_transfer;
               deassert_rdy;
@@ -247,7 +266,7 @@ component main(
     cells{
         vec1_data = seq_mem_d1(32,16,64);
         // Not used currently
-        //vec2_data = seq_mem_d1(32,16,4); 
+        //vec2_data = seq_mem_d1(32,16,4);
         output_data = seq_mem_d1(32,1,0);
 
         curr_addr = std_reg(64); //TODO(nathanielnrn): figure out where this comes from, see generated toplevel and kernel interface requirements below.

[rachitnigam]

I think this was probably caused by #859

[rachitnigam]

@nathanielnrn had a synchronous discussion about this and we noticed a couple of problems:

1. The main.go signal isn't asserted in the cocotb harness
2. The main.reset is not asserted which is needed to reset the state inside the control registers

The fix was to add the following to the top of the harness:

    # Assert reset for 5 cycles
    main.reset.value = 1
    await ClockCycles(main.clk, 5)  # wait a bit
    main.reset.value = 0

    # Start the execution
    main.go.value = 1

There is a bigger problem to address here: writing a harness to execute Calyx programs is non-trivial because the user needs to reset control state and then correctly assert the go signal. We should document this somewhere.

[rachitnigam]

Also, we should use -s calyx.flags ' --disable-verify' when using fud to disable the assertion generation.

[nathanielnrn]

To elaborate a bit on the Multiple assignment to port issue, this is the result of Icarus simulating xs (which verilator does not do if I understand correctly). These xs end up being true-ish in lines like:

if(~$onehot0({_guard198, _guard197})) begin
    $fatal(2, "Multiple assignment to port ... .");

As @rachitnigam mentioned, one solution to this is making sure that xs aren't simulated by properly restarting the Calyx module on startup, and correctly asserting main's go signal after that.

It is unclear why asking for waveforms causes the simulation to fail here, while just invoking make doesn't seem to encounter this problem. Further more, I was unable to reproduce the Multiple assignment issue as of today. But there doesn't seem to be an underlying Calyx issue so closing this.