Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid counter synthesis #2624

Open
RustamC opened this issue Oct 22, 2024 · 5 comments
Open

Invalid counter synthesis #2624

RustamC opened this issue Oct 22, 2024 · 5 comments

Comments

@RustamC
Copy link

RustamC commented Oct 22, 2024

I've found that Synlig, unlike Yosys1, 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,

  • out <= out + 'h1 gives wrong result,
  • out <= out + 1'h1 or out <= out + 1 give correct results.

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
  1. Display results in GTKWave:
gtkwave rtl.vcd

Pasted image 20241022113735

2. Synthesis & simulation

  1. Synthesis using synlig:
synlig -s ./run.sc
  1. 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
  1. Results in GTKWave:
gtkwave syn.vcd

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

Footnotes

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

@jeras
Copy link

jeras commented Oct 22, 2024

Might be related to #2618

@kamilrakoczy
Copy link
Collaborator

I haven't tried exactly this test case, but it also might be related to: chipsalliance/Surelog#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
Copy link
Author

RustamC commented Oct 23, 2024

@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
  1. AST after simplification:
AST_CONSTANT <./counter.sv:0.0-0.0> [0x55db6df6f210] bits='1111'(4) basic_prep range=[3:0] int=15
  1. 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
Copy link
Collaborator

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: ([email protected]), line:12:14, endln:12:17
                |vpiParent:
                \_operation: , line:12:14, endln:12:23
                |vpiName:out
                |vpiFullName:[email protected]
                |vpiActual:
                \_logic_net: ([email protected]), 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
Copy link
Author

RustamC commented Oct 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants