I was reading the paper and noted the following ...

(9) Testing Suite. The STL shall have unit tests for each module.

Is this the testing suite?

... is the above assertion correct (for each module)?

We should add a PPA regression to track PPA across synthesis tools, versions and pull requests.

Needed for BlackParrot and other 64 bit processors. Support looks to be mostly there, items I see:

1. modify read/write mask
2. modify test makefiles to accept data width as a param
3. modify trace_gen.py script to output 64 bit traces

Don't know if it's worth addressing - or if it can be - but here's the warning:

WARNING: [Synth 8-1921] elaboration system task fatal violates IEEE 1800 syntax [/mnt/bsg/diskbits/dustinar/bsg_bladerunner/bsg_f1_621ec48/cl_manycore/build/src_post_encryption/bsg_dlatch.v:10]```

For consistency, we should put this file in the bsg_manycore repo with the other manycore link converters.

https://github.com/bespoke-silicon-group/basejump_stl/blob/de58dc4df30f64ca2303323d04741190cbd0b812/bsg_cache/bsg_manycore_link_to_cache.v

...to match design intent

When you try to compile bsg_mem/bsg_mem_1rw_sync_mask_write_bit_synth.v with Verilator, you get this error:

%Error-BLKLOOPINIT: /home/petrisko/bitbucket/test/pre-alpha-release/basejump_stl/bsg_mem/bsg_mem_1rw_sync_mask_write_bit_synth.v:42: Unsupported: Delayed assignment to array inside for loops (non-delayed is ok - see docs)

Verilator does not like this construct

 always_ff @(posedge clk_i)
   if (v_i & w_i)
       for (i = 0; i < width_p; i=i+1)
         if (w_mask_i[i])
           mem[addr_i][i] <= data_i[i];

This would work functionally, but may not synthesize to the same, depending on compiler quality.

 always_ff @(posedge clk_i)
   if (v_i & w_i)
     mem[addr_i] <= (~wmask_i & mem[addr_i]) | (w_mask_i & data_i); 

There were comments for improvements in PR #111 (merged) that need to be fixed.

There are too many modules in the library. Sometimes a simple function can be implemented by two or more modules. I cannot find a doc to help me to choose the proper module. It is better to offer

1. A user manual to specify the interface and function of each module. It can also help the user to understand the differences between similar module.
2. A design specification to explain the principle of each module. It maybe useful for senior designer.

if els_p=1, data_o should be data_i, regardless of sel_i.

These should ideally use hardened synchronizer flops, but this is not a major issue for this chip.

Would be nice to provide a nice, clean api for hooking into trace-replay (rather than everyone rolling their own python generators). I'm picturing a 1 file python library that you can call with your own higher level test-generator

In order to support RV64F

1. Floating point divide (most likely implemented as FP inversion and then multiplication)
2. Floating point square root
3. Floating point classify (decode types of NaNs and subnormal numbers)

This module uses a full twofer FIFO to hold the packet even though data is draining out serially. Much like SIPO, this can be improved by using one element FIFOs except for the top flit which is two element. This way, it can accept a new word even if it is uncertain whether the last flit will be dequed. The logic is a little more complicated because the location of the last flit changes.

M

a. add default-on but disabl-able warning that detects when we wanted to deque data but data is coming in too fast.

b. add max_conflict_run_p parameter. (lesser priority, since we don't currently have a need for this)

The counter pointer in this module can not hit the value of power of 2.

The first will bring the line into the cache and lock it. The second will unlock the line but not evict it.

Although the async FIFO ensures that the SRAM is never written in the same place as it currently being read, in a synthesized SRAM, it is theoretically possible that changes resulting from the write can propagate to the output either because the FIFO is empty and a write is being done to where the read is currently pointing or simply because the MUX that is MUXing among register items is glitchy. It is thus theoretically possible, depending on synthesis results, for async writes to cause temporary glitches at inopportune times that violate setup and hold time of the destination flops. A more stable solution would be to add a hardened data gate that prevents data changes from propagating when a read is not being performed.

Assertions should be triggered in negedge of clock to avoid races.

Can we convert the wormhole routers from a ready-valid interface to bsg_ready_and_link_sif links? That would make it much easier to stitch together a mesh in the standard way. Preferably we can change this quickly before we make the off-chip network in BlackParrot, so that we don't have to redo work. Also, can we change local_x_cord_i to my_x_i for consistency?

Current

  (input clk_i
  ,input reset_i
  
  // Configuration
  ,input [x_cord_width_p-1:0] local_x_cord_i
  ,input [y_cord_width_p-1:0] local_y_cord_i
  
  // Input Traffics
  ,input [dirs_lp-1:0] valid_i // early
  ,input [dirs_lp-1:0][width_p-1:0] data_i 
  ,output [dirs_lp-1:0] ready_o // early
  
  // Output Traffics
  ,output [dirs_lp-1:0] valid_o // early
  ,output [dirs_lp-1:0][width_p-1:0] data_o 
  ,input [dirs_lp-1:0] ready_i // early

mesh_router

   (
    input clk_i
    , input reset_i

    , input  [dirs_lp-1:0][bsg_ready_and_link_sif_width_lp-1:0] link_i
    , output [dirs_lp-1:0][bsg_ready_and_link_sif_width_lp-1:0] link_o

    , input [x_cord_width_p-1:0] my_x_i
    , input [y_cord_width_p-1:0] my_y_i
    );

Is this router in use anywhere? Are we ready to deprecate it?

`define declare_bsg_then_ready_link_sif_s(in_data_width,in_struct_name)\
   typedef struct packed {                                                \
      logic       v;                                                      \
      logic       then_ready_rev;                                         \
      logic [in_data_width-1:0] data;                                     \
  } in_struct_name

In macros like above, data fields in NOC links are more often than not packets and packets are almost always structs. It's hard to interpret the data section as is, while debugging. Wouldn't it be better to take a "data type" argument instead of data width?

I'm proposing we could replace in_data_width with something like data_type_s

`define declare_bsg_then_ready_link_sif_s(data_type_s,in_struct_name)\
   typedef struct packed {                                                \
      logic       v;                                                      \
      logic       then_ready_rev;                                         \
      data_type_s data;                                     \
  } in_struct_name

@proftaylor @tommydcjung @dpetrisko What do you think?

%Warning-STMTDLY: /home/petrisko/bitbucket/pre-alpha-release/basejump_stl/bsg_noc/bsg_wormhole_router_generalized.v:45: Unsupported: Ignoring delay on this delayed statement.
%Warning-STMTDLY: Use "/* verilator lint_off STMTDLY */" and lint_on around source to disable this message.

I think we need `ifndef VERILATOR surrounding this. This will just get rid of the assertion in Verilator. Otherwise, the assertion may fire only in verliator testbenches. Another solution would be to find a way to mask the assertion when not useful without delay (since delay is awful for many other reasons).

Right now, if you run many of the tests in testing (testing/bsg_noc/bsg_wormhole_router_adapter_in), they expect you to have vcs on your PATH, along with license servers set up, etc.

We should have a public bsg_cadenv repo where users can set up the CAD variables needed and then Makefiles in BaseJump STL should use the correct variables and make no assumptions about environment.

This could be a shorter term solution to #36, or possibly orthogonal. I don't know how license servers work with FuseSoC

pipeline_p=1 : 1-stage pipeline
pipeline_p=0 : no pipeline

This would be a multiplexed FIFO implementation, which leverages a single hardened SRAM to store state. The idea is that on input, you have a tag which is which channel it corresponds to. And somehow you have the ability to pull data based on what channel you want to access.

A lot of different implementations, TBD what the right interface is.

Would use hardened 1R1W and a bsg_dff_en_bypass so that it can grab the read element out. Need to be careful about simultaneous read and write to same address of the 1R1W.

This will allow us to portably build the tests for different verilog simulators so that people can more easily use the tests.

Should be able to instantiate repeaters between nodes of the mesh, for long links.

proposed parameter:

parameter int repeater_depth_lp [max_y_dim_p*max_x_dim_p:0] = 
  '{1, 0, 0, 2, 3, 0}

Hi, I'm trying to evaluate the bsg_mem_banked_crossbar for a project I'm working on.
I can't seem to get the test to work correctly.
Using modelsim 2015, I only get

Error: Error at time: (time), no transaction in a cycle

I changed

to

bsg_mem_banked_crossbar #( .bank_size_p  (bank_size_lp)
                            ,.num_ports_p  (ports_lp)
                            ,.num_banks_p  (banks_lp)
                            ,.rr_lo_hi_p(0) 
                            ,.data_width_p (data_width_lp)
                           ) UUT
                           ( .clk_i   (clk)
                            ,.reset_i (reset)
                            ,.reverse_pr_i('0)
                            ,.v_i     (test_input_v)
                            ,.w_i     (test_input_w)
                            ,.addr_i  (test_input_addr)
                            ,.data_i  (test_input_data)
                            ,.mask_i  (test_input_mask)
                            ,.yumi_o  (test_output_yumi)
                            ,.v_o     (test_output_v)
                            ,.data_o  (test_output_data)
                           );

and messed around with the makefile to get vsim to stop complaining about missing files:

BSG_MISC_FILES      =   bsg_defines.v bsg_crossbar_o_by_i.v bsg_transpose.v bsg_cycle_counter.v bsg_encode_one_hot.v bsg_mux_one_hot.v bsg_round_robin_arb.v

and

BSG_MEM_FILES       =   bsg_mem_1rw_sync.v bsg_mem_1rw_sync_synth.v bsg_mem_1rw_sync_mask_write_byte.v bsg_mem_1rw_sync_mask_write_byte_synth.v

• Have input be clk_i and not en_i; we want these used in very specific cases (usually a negative clock dlatch, will grab and pass through the new value at the second half of the cycle).
• Use always_latch and not always_ff
• Add %m warning if it is instantiated; and i_know_this_is_a_bad_idea_p which disables the warning.

to be consistent with wormhole_router_generalized

https://github.com/bespoke-silicon-group/basejump_stl/blob/master/bsg_noc/bsg_wormhole_router_adapter_in.v

https://github.com/bespoke-silicon-group/basejump_stl/blob/master/bsg_noc/bsg_wormhole_router_adapter_out.v

I'm sure there are more, but these come up during BP compile

"/mnt/bsg/diskbits/petrisko/scratch/pre-alpha-release/basejump_stl/bsg_misc/bsg_mux_one_hot.v",
26
"/mnt/bsg/diskbits/petrisko/scratch/pre-alpha-release/basejump_stl/bsg_mem/bsg_mem_1rw_sync.v",
35
"/mnt/bsg/diskbits/petrisko/scratch/pre-alpha-release/basejump_stl/bsg_mem/bsg_mem_1rw_sync_mask_write_bit.v",
35
"/mnt/bsg/diskbits/petrisko/scratch/pre-alpha-release/basejump_stl/bsg_misc/bsg_priority_encode_one_hot_out.v",
24
"/mnt/bsg/diskbits/petrisko/scratch/pre-alpha-release/basejump_stl/bsg_misc/bsg_encode_one_hot.v",

This file is obsoleted by bsg_counter_up_down.v, which has the identical functionality and code; this file should be removed after dependent files are coded to use that other module.

(x-post to BSG Manycoree)
It seems unnecessary to have $display statements enabled for every simulation run. Printing information on every run can slow the simulator down, or clog with unnecessary information.

In BSG F1 we have a printing header. bsg_pr_debug statements are only enabled when the DEBUG macro is defined. This can be defined globally, or on a per-file basis.

Instead of putting raw $display statements in BaseJump Code, it would be cool if we could wrap $display in a macro that is only enabled when DEBUG is defined. Like in C/C++ we could enable this on a per-file basis (by putting define DEBUG include "bsg_basejump_printing.vh") or globally when compiling the simulation executable

Dominic @ Google had an infrastructure that they suggested.

Can't use genvar in this context (not in a generate statement). I'm not sure what the correct behavior is but it seems like you might just want to get rid of the [i].

Fix 1: (bare #1000 statement). Actual best practice for this is $assertoff ...delay... $asserton in the testbench, but this fix will work, too

`ifndef SYNTHESIS
`ifndef VERILATOR
    wire [dirs_lp-1:0][dirs_lp-1:0] matrix_out_in_transpose;

    bsg_transpose #(.width_p(dirs_lp),.els_p(dirs_lp)) tr (.i(routing_matrix_p[0])
                                                          ,.o(matrix_out_in_transpose)
                                                          );
    initial
      begin
        #1000;
        assert (routing_matrix_p[1] == matrix_out_in_transpose)
          else $error("inconsistent matrixes");
      end
`endif
`endif

But even with that, I get this error:

http://bjump.org/stl/

Redirects to the bitbucket repo