Resolve slang errors in VTR benchmarks

[petergrossmann21]

This PR attempts to resolve errors and warnings encountered when running slang on the VTR benchmarks.

In broad strokes, there are three types of errors that were fixed:

1. Eliminating defparam statements in favor of a more contemporary parameter override syntax
2. Making output ports type reg where needed because they are used in always blocks.
3. Reordering wire declarations and assign statements so that the wire declaration for a signal always precedes its use in an assign statement.

Every attempt was made to make these edits minimally invasive, up to and including leaving odd whitespace intact along with any number of other syntax/style choices that would not normally be acceptable in professional-grade HDL code.

In addition to these changes, a redundant compiler directive was converted to a parameter in mkDelayWorker32B.v to attempt to preserve design intent. Depending upon exactly how yosys was processing the redundant directive, this may or may not change synthesis results for this benchmark.

Related Issue

This work supports #3169

Motivation and Context

The goal of this PR is to make the circuits slang-clean so they can be reliably used with that plugin in yosys.

How Has This Been Tested?

Initial testing has been done only running slang locally on each Verilog file and by obtaining the following result

/tools/vpr/vtr-verilog-to-routing-fix_benchmark_slang/vtr-verilog-to-routing/vtr_flow/tasks/regression_tests/vtr_reg_nightly_test3/vtr_reg_qor_chain$ ../../../../../vtr_flow/scripts/run_vtr_task.py . -check_golden

regression_tests/vtr_reg_nightly_test3/vtr_reg_qor_chain...[Pass]

Types of changes

• Bug fix (change which fixes an issue)
• New feature (change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

[loglav03]

I've begun testing the fixed verilog with my setup for yosys-slang and I'm getting failing benchmarks.

and_latch.v - PASSED
arm_core.v - FAILED at VPR terminated with signal 6
boundtop.v - FAILED at read_slang --- error: parameter 'ADDR_WIDTH' does not exist in 'single_port_ram'
# (.ADDR_WIDTH(10), .DATA_WIDTH(32))
ch_intrinsics.v FAILED at read_slang with similar errors as boundtop
LU8PEEng.v FAILED at read_slang with similar errors as boundtop

I stopped here because I felt like most other benchmarks would be getting similar errors. I'm not sure, but maybe it is because I disabled the UndrivenPass for slang #3169? The UndrivenPass was causing the vtr_primitives.v to error out.

[petergrossmann21]

@loglav03 Can you do the following:

1. Confirm which file in the VTR repository you are getting your primitives from? There are four copies of vtr_primitives.v in the repo that I am working with. All have the ADDR_WIDTH parameter defined for single_port_ram
2. Provide an indicator of which test command you ran to produce the failures?

[petergrossmann21]

One small additional update. After installing the slang plugin from source on top of Yosys 0.54 and hiding the specify blocks in vtr_flow/primitives.v behind `ifdef wrappers, I was able to do the following simple experiment:

• execute Yosys from VPR repo root directory
• run the following commands at the Yosys tcl prompt:

plugin -i slang
read_slang vtr_flow/primitives.v vtr_flow/benchmarks/verilog/boundtop.v 

This command sequence completed successfully. So I think we need to focus on what issues in the test setup are interfering with a successful parse.

[petergrossmann21]

Update: thanks to the traffic on #3181, I was able to quickly find my way to a portion of the testing that was being done, and use it to design a follow-up experiment. From a yosys command line, I did the following:

plugin -i slang
read_verilog -nomem2reg vtr_flow/primitives.v
read_slang vtr_flow/benchmarks/verilog/boundtop.v

Note that the parmys vtr_primitives.v file used in and vtr_flow/primitives.v file are the same, plus or minus a timescale directive. The parmys vtr_primitives.v is used in vtr_flow/misc/yosys/synthesis.tcl, where @loglav03 is adding the slang support.

When I execute the above commands, I obtain the following errors similar to those reported by @loglav03

vtr_flow/benchmarks/verilog/boundtop.v:1652:7: error: parameter 'ADDR_WIDTH' does not exist in 'single_port_ram'
  # (.ADDR_WIDTH(10), .DATA_WIDTH(32))
      ^
vtr_flow/benchmarks/verilog/boundtop.v:1652:24: error: parameter 'DATA_WIDTH' does not exist in 'single_port_ram'
  # (.ADDR_WIDTH(10), .DATA_WIDTH(32))

Something is preventing the read_slang command from seeing that the single_port_ram and dual_port_ram blocks are parameterized unless the primitives are read in as part of the same read_slang command as the benchmark. I've tried a few different things to try to resolve this, but don't have a working recipe just yet.

Note that the parameters are present in the modules after executing read_verilog, as evidenced by the following yosys output:

yosys> chparam -list
dual_port_ram:
  ADDR_WIDTH
  DATA_WIDTH
single_port_ram:
  ADDR_WIDTH
  DATA_WIDTH
multiply:
  WIDTH
adder:
  WIDTH
mux:
fpga_interconnect:
DFF:
  INITIAL_VALUE
LUT_K:
  K
  LUT_MASK

[petergrossmann21]

After more digging, I found an open issue in yosys-slang that describes similar results to what I am seeing:

povik/yosys-slang#72

Note that the proposed workaround involves always reading the library verilog and the user verilog with the same command. I'm not clear on whether that solution is workable for the VTR tests or not, @loglav03: thoughts?

[povik]

@petergrossmann21 I confirm read_slang won't see the parameters on modules loaded into Yosys with read_verilog. If you require to mix read_verilog and read_slang, the following workaround is available:

First you load the modules as required with read_verilog.

Second you load the user design with read_slang, at the same time you describe the pre-existing modules as blackboxes. E.g. for your dual_port_ram primitive you would add the following to the read_slang file list:

`celldefine
module dual_port_ram #(
    parameter ADDR_WIDTH = 1,
    parameter DATA_WIDTH = 1
) (
    input clk,

    input [ADDR_WIDTH-1:0] addr1,
    input [ADDR_WIDTH-1:0] addr2,
    input [DATA_WIDTH-1:0] data1,
    input [DATA_WIDTH-1:0] data2,
    input we1,
    input we2,
    output reg [DATA_WIDTH-1:0] out1,
    output reg [DATA_WIDTH-1:0] out2
);
endmodule
`endcelldefine

This way slang understands the parameters and their influence over port widths which is critical for correct elaboration.

When involving blackboxes make sure to specify the top with --top (related to an issue povik/yosys-slang#115). Let me know if you have trouble with this method.

[povik]

One point which surprises me about your scripts is that you apply read_verilog to your primitives instead of read_verilog -lib. This means the modules are read including their behavioral content as if they were a user design. (The workaround from above would work with read_verilog -lib too.)

[petergrossmann21]

@loglav03 @vaughnbetz What are your thoughts about reducing the proposed workaround to practice? Are there any issues precluding doing as suggested in the synthesis scripts?

[vaughnbetz]

The workaround seems fine to me. I'd just check the resource count and cpd results before and after the changes to ensure we haven't accidentally broken something. If the scripts are using an odd command or an order that doesn't always work we should change them.

[loglav03]

@petergrossmann21 It's worth a shot. I don't believe there's anything blocking us from trying this. I already attempted the workaround by adding the single_port and duel_port ram primitives described as blackboxes to a seperate file and added it in as an include file to be added to the file list along with the user verilog.

This resulted in this: ERROR: Attempted to add new module named '\dual_port_ram', but a module by that name already exists
But this is most likely caused by me not having it set up correctly.

[povik]

This resulted in this: ERROR: Attempted to add new module named '\dual_port_ram', but a module by that name already exists

@loglav03 The key part is that for slang the module is wrapped in

`celldefine
...
`endcelldefine

Alternatively it should have the (* blackbox *) attribute. With this change you shouldn't be getting the error above.

[povik]

The "module by that name already exists" error might have also been caused by leaving out --top <module-name> from the command line. This is pending a fix in yosys-slang.

[loglav03]

@petergrossmann21 I just tried to run read_slang on both the vtr_primitives.v and ch_intrinsics.v (include file) and it gave me this error:

Source line vtr_primitives.v:156:5:     specify
ERROR: Feature unimplemented at /home/llavign1/Gits/vtr-clone/libs/EXTERNAL/yosys-slang/src/slang_frontend.cc:2854, see AST and code line dump above

It looks like you previously has success with running

plugin -i slang
read_slang vtr_flow/primitives.v vtr_flow/benchmarks/verilog/boundtop.v 

So I'm not sure why yours passed and mine didn't since it seems like yosys-slang doesn't currently support specify blocks. I ran my test through the synthesis.tcl script by running run_vtr_flow.py on my own benchmark directory with vtr_primitives.v and ch_intrinsics.v

I think this means we need to only be calling read_verilog on vtr_primitives and will need to go down the workaround route that was proposed. I will continue trying things out and see if I can get something working.

[loglav03]

The "module by that name already exists" error might have also been caused by leaving out --top <module-name> from the command line. This is pending a fix in yosys-slang.

@povik @petergrossmann21 - I was able to get this working without throwing the "module by that name already exists" error and shows

2. Executing SLANG frontend.
Top level design units:
    dual_port_ram
    single_port_ram

However, I'm running into the same issue I was having above. These modules get read by read_slang and throw

Source line vtr_blackboxes.v:60:5:     specify
ERROR: Feature unimplemented at /home/llavign1/Gits/vtr-clone/libs/EXTERNAL/yosys-slang/src/slang_frontend.cc:2854, see AST and code line dump above

I'm not experienced with verilog, so I'm not sure if there's a possible workaround with the specify blocks that won't break anything.

[povik]

Hi @loglav03

If you're getting this output:

2. Executing SLANG frontend.
Top level design units:
    dual_port_ram
    single_port_ram

I think you haven't specified the top with --top.

[loglav03]

This is what I have set up in the tcl script:

set readfile [file join [pwd] "filelist.txt"]
#Writing names of circuit files to file list
build_filelist {XXX} $readfile
puts "Using Yosys read_slang command"
read_slang --top single_port_ram --top dual_port_ram -C $readfile

The file that defines the single and dual port ram modules as blackboxes is included in the $readfile

[povik]

Got it. The module specified with --top should be the user design top.

[loglav03]

@povik Thank you for taking the time to help with this. @petergrossmann21 I managed to get the synthesis script set up in a way that passes verilog benchmarks that were previously failing with yosys-slang.

When slang is enabled, it does this in the synthesis script where -DNO_SPECIFY is a macro that prevents slang from reading the specify blocks in vtr_primitives. So, with this macro, I was able to read the user design verilog along with vtr_primitives with the same read_slang command. This resulted in the benchmarks (listed below) passing.

For the test I used, I made my own copy of the vtr_reg_nightly_test3/vtr_reg_qor with your fixed benchmarks substituted in for the old ones to test these benchmarks. It doesn't cover all of the ones you modified, but I would say the other ones would probably pass as well if we were to test them. I haven't checked the golden results with these yet so that's something I still have to do.

Modified portion of synthesis.tcl:

set readfile [file join [pwd] "filelist.txt"]
#Writing names of circuit files to file list
build_filelist {XXX} $readfile
puts "Using Yosys read_slang command"
read_slang -DNO_SPECIFY -C $readfile

Example of the guard on specify blocks:

`ifndef NO_SPECIFY
    specify
        (in *> out) = "";
    endspecify
`endif // NO_SPECIFY

Benchmarks that passed:

mcml.v
LU32PEEng
LU8PEEng
stereovision3
or1200.v
raygentop.v
mkSMAdapter4B.v
mkPktMerge.v
mkDelayWorker32B.v
ch_intrinsics.v
boundtop.v
bgm.v 
arm_core.v

[loglav03]

Update on the above workaround

The benchmarks had passed through the entire flow but they are failing a large amount of golden results (if not all of them). So this method doesn't seem like its feasible, and we will need find another workaround.

[povik]

Can you provide details on the nature of the failure? Is this a QoR check or a functional check which is failing?

[loglav03]

These are QoR failures

[povik]

Do I understand the screenshot correctly that the result is much smaller than expected? It looks like the design did not get read in correctly and got optimized out, or the wrong module is getting synthesized.

[loglav03]

You are correct these results are much smaller than expected. There's even some that are showing relative value 0.0 outside of range [0.9,1.1]. I assumed we're getting these results because of the specify blocks I have set to not be read by slang in vtr_primitives to avoid throwing the "specify ERROR: Feature unimplemented" errors. Below are the specify blocks in single and dual port ram modules that I have guarded against slang.

Single port ram:

localparam MEM_DEPTH = 2 ** ADDR_WIDTH;

reg [DATA_WIDTH-1:0] Mem[MEM_DEPTH-1:0];

`ifndef NO_SPECIFY
    specify
        (clk*>out)="";
        $setup(addr, posedge clk, "");
        $setup(data, posedge clk, "");
        $setup(we, posedge clk, "");
        $hold(posedge clk, addr, "");
        $hold(posedge clk, data, "");
        $hold(posedge clk, we, "");
    endspecify
`endif // NO_SPECIFY
   
    always@(posedge clk) begin
        if(we) begin
            Mem[addr] = data;
        end
    	out = Mem[addr]; //New data read-during write behaviour (blocking assignments)
    end

Dual port ram:

localparam MEM_DEPTH = 2 ** ADDR_WIDTH;

reg [DATA_WIDTH-1:0] Mem[MEM_DEPTH-1:0];

`ifndef NO_SPECIFY
    specify
        (clk*>out1)="";
        (clk*>out2)="";
        $setup(addr1, posedge clk, "");
        $setup(addr2, posedge clk, "");
        $setup(data1, posedge clk, "");
        $setup(data2, posedge clk, "");
        $setup(we1, posedge clk, "");
        $setup(we2, posedge clk, "");
        $hold(posedge clk, addr1, "");
        $hold(posedge clk, addr2, "");
        $hold(posedge clk, data1, "");
        $hold(posedge clk, data2, "");
        $hold(posedge clk, we1, "");
        $hold(posedge clk, we2, "");
    endspecify
`endif // NO_SPECIFY
   
    always@(posedge clk) begin //Port 1
        if(we1) begin
            Mem[addr1] = data1;
        end
        out1 = Mem[addr1]; //New data read-during write behaviour (blocking assignments)
    end

    always@(posedge clk) begin //Port 2
        if(we2) begin
            Mem[addr2] = data2;
        end
        out2 = Mem[addr2]; //New data read-during write behaviour (blocking assignments)
    end

It might also be because I don't have it reading the vtr_primitives correctly with the user design? I get this at the start of the output for yosys, and it looks like its reading it properly, but I'm not sure what the warning is for ch_intrinsics.v. Also vtr_primitives is being read by read_slang along with the user design in the same command, so I'm not sure if it would need an equivalent -lib. Maybe that could be a reason?

1. Executing SLANG frontend.
ch_intrinsics.v:49:9: warning: non-edge sensitivity on a signal will be synthesized as @* sensitivity
always @( tag or memory_controller_address or memory_controller_write_enable or memory_controller_in)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ch_intrinsics.v:278:10: warning: non-edge sensitivity on a signal will be synthesized as @* sensitivity
always @(cur_state)
         ^~~~~~~~~
Top level design units:
    DFF
    LUT_K
    adder
    dual_port_ram
    fpga_interconnect
    memset
    multiply
    mux

[povik]

@loglav03 Based on this part in the output

Top level design units:
    DFF
    LUT_K
    adder
    dual_port_ram
    fpga_interconnect
    memset
    multiply
    mux

I see you are still not setting the top. Please pass a --top argument to read_slang. E.g. for this design I'm guessing --top memset.

[loglav03]

So I ran this in the synthesis.tcl read_slang -DNO_SPECIFY --top memset -C $readfile where the full vtr_primitives.v file is included in the readfile.

This passed through the entire yosys stage, but when it reached the VPR stage (packing, placing, routing) it threw this error:

# Building complex block graph
Warning 1: io[0].clock[0] unconnected pin in architecture.
# Building complex block graph took 0.01 seconds (max_rss 24.3 MiB, delta_rss +5.5 MiB)
Circuit file: ch_intrinsics.pre-vpr.blif
# Load circuit
# Load circuit took 0.00 seconds (max_rss 26.4 MiB, delta_rss +2.1 MiB)
Error 1: 
Type: Blif file
File: ch_intrinsics.pre-vpr.blif
Line: 171
Message: Only rising edge latches supported

Not sure what this means or how it can be fixed. @vaughnbetz @AlexandreSinger @povik - Any thoughts?