bespoke-silicon-group / basejump_stl

BaseJump STL: A Standard Template Library for SystemVerilog
http://bjump.org/
Other
525 stars 99 forks source link

How can we use a basejump_stl component as a top-level module? #613

Closed stevenmburns closed 1 year ago

stevenmburns commented 1 year ago

I want to use cocotb to test some basejump_stl components. It assumes that the design under test is a top level module. But many components, in particular, say bsg_abs has a back-tick define declaring this module as abstract, so I can't use it as a top-level module. I think this is deliberate, but I want it to be a top-level module in my use-case. Any thoughts on how to elegantly disable the effect of the BSG_ABSTRACT_MODULE macro?

/**
 *  bsg_abs.v
 *
 *  calculate absolute value of signed integer.
 *
 *  @author Tommy Jung
 */

`include "bsg_defines.v"

module bsg_abs #( parameter `BSG_INV_PARAM(width_p) )
(
  input [width_p-1:0] a_i
  ,output logic [width_p-1:0] o
);

  assign o = a_i[width_p-1]
    ? (~a_i) + 1'b1
    : a_i;

endmodule

`BSG_ABSTRACT_MODULE(bsg_abs)
dpetrisko commented 1 year ago

Hi, can you not set the toplevel module of cocotb using TOPLEVEL? https://docs.cocotb.org/en/stable/building.html

image
stevenmburns commented 1 year ago

The problem is not in Cocotb, but (I least I think) in basejump_stl deliberately not allowing components to be used as toplevel modules. Here is the error when I set toplevel as you described [verilator is the simulator] (this used to work a year or so ago):

INFO     cocotb:simulator.py:315 Running command: perl /nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/verilator_install/bin/verilator -cc --exe -Mdir /nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/pbt/sim_build -DCOCOTB_SIM=1 --top-module bsg_abs --vpi --public-flat-rw --prefix Vtop -o bsg_abs -LDFLAGS -Wl,-rpath,/nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/cocotb/cocotb/libs -L/nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/cocotb/cocotb/libs -lcocotbvpi_verilator -pvalue+width_p=8 -I/nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/basejump_stl/bsg_misc /nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/cocotb/cocotb/share/lib/verilator/verilator.cpp /nfs/site/disks/scl.work.50/ash/users/smburns/cocotb-basejump_stl/basejump_stl/bsg_misc/bsg_abs.v
ERROR    cocotb:simulator.py:291 %Error: Specified --top-module 'bsg_abs' isn't at the top level, it's under another cell 'bsg_abs__abstract'
ERROR    cocotb:simulator.py:291 %Error: Exiting due to 1 error(s)

The bsg_abs__abstract cell is being generated by the macro BSG_ABSTRACT_MODULE. I can get around this by adding a wrapper module around the basejump_stl component, but I wonder if there is something else to do. It also works fine if I remove the call to the BSG_ABSTRACT_MODULE macro, but I don't want to be changing the library to run these tests.

stevenmburns commented 1 year ago

The example is here if you want to try it yourself: https://github.com/stevenmburns/cocotb-basejump_stl/pull/11/files# The test_abs_no_wrapper.py case fails. The others pass because I've added wrapper mdoules.

dpetrisko commented 1 year ago

Which version of verilator are you using? This seems related to:

https://github.com/verilator/verilator/issues/3014 https://github.com/verilator/verilator/pull/3026

which provide the ability to use toplevels from non-toplevel components?

dpetrisko commented 1 year ago

Alright, I see from your link that you're using 4.106. These features came in 4.210. Is there a cocotb limitation to upgrading?

stevenmburns commented 1 year ago

Yes, unfortunately cocotb is stuck on version 4.106 of verilator.

https://github.com/cocotb/cocotb/issues/2300

(This seems like an especially hard problem to fix.)

dpetrisko commented 1 year ago

Wow, that is quite a well-discussed issue. We tend to dislike patching things for anything but the latest version of simulators/synthesis tools, but I would quite like to see cocotb+verilator compatibility.

diff --git a/bsg_misc/bsg_defines.v b/bsg_misc/bsg_defines.v
index 98726440..cf5adb00 100644
--- a/bsg_misc/bsg_defines.v
+++ b/bsg_misc/bsg_defines.v
@@ -23,12 +23,14 @@
 //    module name. Such an instance is called a top-level instance."
 //

-`define BSG_ABSTRACT_MODULE(fn) \
-    /*verilator lint_off DECLFILENAME*/ \
-    /*verilator lint_off PINMISSING*/ \
-    module fn``__abstract(); if (0) fn not_used(); endmodule \
-    /*verilator lint_on PINMISSING*/ \
-    /*verilator lint_on DECLFILENAME*/
+//`define BSG_ABSTRACT_MODULE(fn) \
+//    /*verilator lint_off DECLFILENAME*/ \
+//    /*verilator lint_off PINMISSING*/ \
+//    module fn``__abstract(); if (0) fn not_used(); endmodule \
+//    /*verilator lint_on PINMISSING*/ \
+//    /*verilator lint_on DECLFILENAME*/
+
+`define BSG_ABSTRACT_MODULE(fn)

For now, I would suggest applying this patch to bsg_defines, rather than modifying the rest of your source code. However, I caution you that this may be a rabbit hole of figuring out which BaseJump STL modules include each other and making sure that tests lists only have each file and no more

taylor-bsg commented 1 year ago

https://github.com/verilator/verilator/issues/2778#issuecomment-1320285567

On Tue, Nov 29, 2022 at 12:10 AM Dan Petrisko @.***> wrote:

Wow, that is quite a well-discussed issue. We tend to dislike patching things for anything but the latest version of simulators/synthesis tools, but I would quite like to see cocotb+verilator compatibility.

diff --git a/bsg_misc/bsg_defines.v b/bsg_misc/bsg_defines.v index 98726440..cf5adb00 100644 --- a/bsg_misc/bsg_defines.v +++ b/bsg_misc/bsg_defines.v @@ -23,12 +23,14 @@ // module name. Such an instance is called a top-level instance." //

-`define BSG_ABSTRACT_MODULE(fn) \

  • /verilator lint_off DECLFILENAME/ \
  • /verilator lint_off PINMISSING/ \
  • module fn``__abstract(); if (0) fn not_used(); endmodule \
  • /verilator lint_on PINMISSING/ \
  • /verilator lint_on DECLFILENAME/ +//`define BSG_ABSTRACT_MODULE(fn) \ +// /verilator lint_off DECLFILENAME/ \ +// /verilator lint_off PINMISSING/ \ +// module fn``__abstract(); if (0) fn not_used(); endmodule \ +// /verilator lint_on PINMISSING/ \ +// /verilator lint_on DECLFILENAME/
  • +`define BSG_ABSTRACT_MODULE(fn)

For now, I would suggest applying this patch to bsg_defines, rather than modifying the rest of your source code. However, I caution you that this may be a rabbit hole of figuring out which BaseJump STL modules include each other and making sure that tests lists only have each file and no more

— Reply to this email directly, view it on GitHub https://github.com/bespoke-silicon-group/basejump_stl/issues/613#issuecomment-1330240265, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEFG5AGLOYIBG2LU6QJVBNTWKW27VANCNFSM6AAAAAASN4BJD4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

stevenmburns commented 1 year ago

I'll go with the fork of verilator. Thanks everyone.