chipsalliance / synlig

SystemVerilog support for Yosys
Apache License 2.0
163 stars 21 forks source link

Invalid counter synthesis #2624

Open RustamC opened 5 days ago

RustamC commented 5 days ago

I've found that Synlig, unlike Yosys[^1], generates an incorrect netlist for simple counter if the increment value is an integer literal constant specified in hexadecimal format without optional size constant.

For example,

I've tested it using Sky130 library. The test example with testbench & syn script: counter.tar.gz (modified version of this example).

Steps to reproduce:

0. Unpack the archive with the test example

tar xzvf counter.tar.gz ./
cd ./counter

1. Original RTL Verilog simulation:

  1. Run Icarus Verilog:
    iverilog -g2012 -s tb_counter ./counter.sv tb_counter.v -o counter.rtl.out
    vvp ./counter.rtl.out
    mv dump.vcd rtl.vcd
  2. Display results in GTKWave:
    gtkwave rtl.vcd

    Pasted image 20241022113735

2. Synthesis & simulation

  1. Synthesis using synlig:
    synlig -s ./run.sc
  2. Simulation:
    iverilog -g2012 -s tb_counter ./syn/counter.syn.v tb_counter.v ./verilog/primitives.v ./verilog/sky130_fd_sc_hd.v -o counter.syn.out -DFUNCTIONAL -DUNIT_DELAY=#1
    mv dump.vcd syn.vcd
  3. Results in GTKWave:
    gtkwave syn.vcd

    Pasted image 20241022114140

  4. As you can see, now counter counts in the wrong direction.

[^1]: If you read input counter.sv using Yosys read_verilog -sv command, generated netlist works & simulates correctly.

jeras commented 4 days ago

Might be related to #2618

kamilrakoczy commented 4 days ago

I haven't tried exactly this test case, but it also might be related to: https://github.com/chipsalliance/Surelog/issues/3979

If you want to investigate it further, you can add -debug flag to synlig to dump both surelog UHDM tree and converted yosys AST tree.

Then you can look at uhdmtopModules part of UHDM tree and check vpiSize of this variables.

RustamC commented 4 days ago

@kamilrakoczy, I've added -debug flag to read_systemverilog & got these results:

  1. AST before simplification:
    AST_CONSTANT <./counter.sv:0.0-0.0> [0x55db6df6f210] bits='1111'(4) range=[3:0] int=15
  2. AST after simplification:
    AST_CONSTANT <./counter.sv:0.0-0.0> [0x55db6df6f210] bits='1111'(4) basic_prep range=[3:0] int=15
  3. Verilog AST before/after simplification:
    module counter(clk, rstn, out);
      input [0:0] clk;
      input [0:0] rstn;
      output reg [3:0] out;
      always @(posedge 1'(clk))
        case (|(!(rstn)))
          1'b 1:
            out <= 0;
          default:
            out <= (out)+(4'b 1111);
        endcase
    endmodule

P.S. Should I run Surelog separately to generate UHDM tree? I haven't found a command to dump UHDM tree like in your example using synlig.

kamilrakoczy commented 4 days ago

Oh, sorry, I forgot that now we generate UHDM tree in-memory and UHDM dump isn't generated by default.

Yes, you need to run Surelog separately to create UHDM file:

surelog -parse -elabuhdm counter.sv

after that you can use uhdm-dump (it is built with surelog binary) to display UHDM tree:

uhdm-dump slpp_all/surelog.uhdm

I confirmed, that Surelog expands unsized const to 4'b1111:

          |vpiElseStmt:
          \_assignment: , line:12:7, endln:12:23
            |vpiParent:
            \_if_else: , line:9:5, endln:12:24
            |vpiOpType:82
            |vpiRhs:
            \_operation: , line:12:14, endln:12:23
              |vpiParent:
              \_assignment: , line:12:7, endln:12:23
              |vpiOpType:24
              |vpiOperand:
              \_ref_obj: (work@counter.out), line:12:14, endln:12:17
                |vpiParent:
                \_operation: , line:12:14, endln:12:23
                |vpiName:out
                |vpiFullName:work@counter.out
                |vpiActual:
                \_logic_net: (work@counter.out), line:3:35, endln:3:38
              |vpiOperand:
              \_constant: , line:12:20, endln:12:23
                |vpiDecompile:15
                |vpiSize:4
                |UINT:15
                |vpiConstType:9

I think, according to standard unsized const should have int size and i op j should have length max(L(i), L(j)). Nevertheless, SystemVerilog standard isn't very specific and many tools doesn't follow it completely.

If you think it is a bug, please report it to Surelog, as I think it is a place where (if required) it should be fixed.

RustamC commented 3 days ago

@kamilrakoczy , thank you! I will report it to Surelog.