Importing Black-Parrot into RTLMeter #4
Conversation
|
@gezalore I noted the CI that auto dispatched doesn't run the designs themselves, so doesn't prove much about pulls like this. (Or at least as far as I can tell). Should that be improved? |
Yes, Verialtor could not handle some of the existing designs out of the box, so have omitted that so far, we will get there. |
There was a problem hiding this comment.
Thanks for the great effort @buhraian I know this was not trivial work!
As Wilson said, the CI in this repo does not yet run Verilator itself. I ran it locally with verilator/verilator@e015805 (master), which seems to work fine. Generally if ./rtlmeter run --cases "black-parrot:*" + CI passes I'm happy, but I will check it before merging anyway.
Please instead of designs/black-parrot use designs/BlackParrot, which is consistent with the styling of the project and the existing designs.
As you mentioned, we will need some more longer workloads. Can we also have more, larger configurations? There is some interest in larger many-core simulations, which I think BlackParrot would be fairly suitable for.
I notice there are some log files written from the simulation to the run directory:
work/black-parrot/default/execute-0/hello
├── commit_0.trace
├── DUT-blackparrot.signature
├── stdout_0.txt
├── stdout_global.txt
└── vm_0.trace
They are empty, but please make sure these would not be populated during a run, benchmark file IO is not our goal here.
designs/black-parrot/descriptor.yaml
Outdated
| - repository: https://github.com/black-parrot/black-parrot.git | ||
| revision: 45a28bf96e58f55687f8e09b2521ceada121ad95 | ||
| licenses: | ||
| - LICENSE |
There was a problem hiding this comment.
I think both the basejump_stl and HardFloat submodules used by the BP repo have different licenses and copyright holders from the BP repo itself, if that is indeed the case please add a separate entry and license files for them, so imported code can be properly attributed to its authors. See e.g. OpenPiton for reference: https://github.com/verilator/rtlmeter/blob/main/designs/OpenPiton/descriptor.yaml
designs/black-parrot/descriptor.yaml
Outdated
| - src/bp/bsg_parallel_in_serial_out_passthrough_dynamic.sv | ||
| - src/bp/bsg_serial_in_parallel_out_passthrough_dynamic.sv |
There was a problem hiding this comment.
Maybe should be in the BP section? there are already bsg_ files there.
designs/black-parrot/descriptor.yaml
Outdated
| - src/hardfloat/recFNToIN.v | ||
| - src/hardfloat/recFNToRecFN.v | ||
| - src/hardfloat/HardFloat_specialize.v | ||
| #BP Common Files |
There was a problem hiding this comment.
| #BP Common Files | |
| # BlackParrot Common Files |
designs/black-parrot/descriptor.yaml
Outdated
| - src/basejump_stl/bsg_cache_pkg.sv | ||
| - src/basejump_stl/bsg_noc_pkg.sv | ||
| - src/basejump_stl/bsg_wormhole_router_pkg.sv | ||
| # BP Packages |
There was a problem hiding this comment.
| # BP Packages | |
| # BlackParrot Packages |
designs/black-parrot/descriptor.yaml
Outdated
| - src/bp/bsg_dff_sync_read.sv | ||
| - src/bp/bsg_rom_param.sv | ||
| - src/bp/wrapper.sv | ||
| #Basejump Memory Files |
There was a problem hiding this comment.
| #Basejump Memory Files | |
| # Basejump Memory Files |
Though I'm not sure all the following are "Memory Files"? It's ok to strip these comments if the separation is not entirely clean.
| `ifdef TRACE_ENABLE | ||
| string dumpfile; | ||
| initial | ||
| if ($value$plusargs("bsg_trace=%s", dumpfile)) | ||
| begin | ||
| $display("[BSG-INFO]: Dumping to %s", dumpfile); | ||
| `ifdef FSDB_ENABLE | ||
| $fsdbDumpfile(dumpfile); | ||
| $fsdbDumpvars(0,testbench.wrapper); | ||
| `elsif VPD_ENABLE | ||
| $vcdplusfile(dumpfile); | ||
| $vcdpluson(0,testbench.wrapper); | ||
| $vcdplusautoflushon(); | ||
| `else | ||
| $dumpfile(dumpfile); | ||
| $dumpvars(0,testbench.wrapper); | ||
| `endif | ||
| end | ||
| `endif |
There was a problem hiding this comment.
Please delete this ifdef, to avoid confusion. __rtlmeter_top_include.vh takes care of dump control.
| #include "svdpi.h" | ||
| #include <iostream> | ||
| #include "stdlib.h" | ||
| #include <string.h> | ||
| #include <vector> | ||
|
|
||
| extern "C" void* cosim_init(int hartid, int ncpus, int memory_size, bool checkpoint) { | ||
| return NULL; | ||
| } | ||
|
|
||
| extern "C" int cosim_step(void *handle, | ||
| int hartid, | ||
| uint64_t pc, | ||
| uint32_t insn, | ||
| uint64_t wdata, | ||
| uint64_t status, | ||
| uint64_t cause) { | ||
| return 0; | ||
| } | ||
|
|
||
| extern "C" int cosim_trap(void *handle, | ||
| int hartid, | ||
| uint64_t pc, | ||
| uint32_t insn, | ||
| uint64_t wdata, | ||
| uint64_t status, | ||
| uint64_t cause) { | ||
| return 0; | ||
| } | ||
|
|
||
| extern "C" void cosim_finish(void *handle) { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
If the expectation is that these are never called, please delete the call site and this file, in order to try to avoid C++ dead code. If these are called it's fine to leave them.
designs/black-parrot/tests/post.bash
Outdated
| set -x | ||
| set -e | ||
| # stdout must contain '' | ||
| grep -q "All cores finished! Terminating" _execute/stdout.log |
There was a problem hiding this comment.
I assume this means all cores finished successfully, and e.g. not crashed. Please consider if there is a stronger check you can do here that positively proves correct termination. If this is that it's fine.
|
I have added CI to run cases on PRs and push, please rebase to |
buhraian
force-pushed
the
add-black-parrot
branch
from
5c7138d to
a7fd9e9
Compare
|
The latest commit addresses the following review comments:
|
|
Seems like the error from the failed CI run stems from a recently introduced check in Verilator for dynamically-sized variables (commit |
|
If it's not essential for functionality feel free to remove it |
|
Thanks for the cleanups. I'm landing this on a branch on a trial basis until we can address the remaining outstanding issues. |
This PR serves to include the necessary changes to import the black-parrot design into the RTLMeter framework properly. Currently, I am using
./rtlmeter run --cases black-parrot:default:helloto test the functionality, and I am planning on adding a few more test cases before we finalize the PR as well, just posting this initial version to confirm functionality.