-
Notifications
You must be signed in to change notification settings - Fork 9
Importing BlackParrot into RTLMeter #15
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extraordinary effort on this, the PR looks great! Some minor stuff, mostly nits below.
Additionally please rebase to upstream main
, squash into a single commit, and make sure git show
displays your preferred e-mail in the commit for credit.
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND | ||
ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS | ||
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please add a \n
to the end of all the license files so git
diffs etc don't complain.
origin: | ||
- repository: https://github.com/black-parrot/black-parrot.git | ||
revision: b57d05165bb44a062aaa5b9675af928f847f2fee | ||
licenses: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please strip trailing white space in descriptor.yaml
- src/bp/bp_nonsynth_if_verif.sv | ||
- src/bp/bp_nonsynth_nbf_loader.sv | ||
- src/bp/bp_nonsynth_perf.sv | ||
- src/bp/bp_monitor.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When building with a Verialtor configured with --enale-ccwarn
, gcc 13.3.0 reports an "infinite recursion" warning in bp_monitor.cpp
, however, looks like this file is not required, so hopefully it can just be removed.
In general please double check if there are any other files that are not actually needed for the design (no need to try everything, just whatever you can eyeball).
- -pvalue+max_instr_p=10000000 | ||
- -pvalue+max_cycle_p=10000000 | ||
- -pvalue+stall_cycles_p=100000 | ||
- -pvalue+halt_instr_p=1000 | ||
- -pvalue+heartbeat_instr_p=10000 | ||
- -pvalue+sim_clock_period_p=10 | ||
- -pvalue+sim_reset_cycles_lo_p=0 | ||
- -pvalue+sim_reset_cycles_hi_p=20 | ||
- -pvalue+tb_clock_period_p=2 | ||
- -pvalue+tb_reset_cycles_lo_p=0 | ||
- -pvalue+tb_reset_cycles_hi_p=50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- -pvalue+max_instr_p=10000000 | |
- -pvalue+max_cycle_p=10000000 | |
- -pvalue+stall_cycles_p=100000 | |
- -pvalue+halt_instr_p=1000 | |
- -pvalue+heartbeat_instr_p=10000 | |
- -pvalue+sim_clock_period_p=10 | |
- -pvalue+sim_reset_cycles_lo_p=0 | |
- -pvalue+sim_reset_cycles_hi_p=20 | |
- -pvalue+tb_clock_period_p=2 | |
- -pvalue+tb_reset_cycles_lo_p=0 | |
- -pvalue+tb_reset_cycles_hi_p=50 | |
- -Gmax_instr_p=10000000 | |
- -Gmax_cycle_p=10000000 | |
- -Gstall_cycles_p=100000 | |
- -Ghalt_instr_p=1000 | |
- -Gheartbeat_instr_p=10000 | |
- -Gsim_clock_period_p=10 | |
- -Gsim_reset_cycles_lo_p=0 | |
- -Gsim_reset_cycles_hi_p=20 | |
- -Gtb_clock_period_p=2 | |
- -Gtb_reset_cycles_lo_p=0 | |
- -Gtb_reset_cycles_hi_p=50 |
- src/basejump_stl/bsg_mem_dma.hpp | ||
topModule: testbench | ||
mainClock: testbench.dut_clk | ||
verilatorArgs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add --autoflush
to verilatorArgs
so lines are printed when ready. This makes progress easier to see.
|
||
No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: single closing \n
in file so git
doesn't complain.
initial | ||
begin | ||
if (width_p*els_p > 256) | ||
$display("## %L: instantiating width_p=%d, els_p=%d, read_write_same_addr_p=%d, harden_p=%d (%m)" | ||
,width_p,els_p,read_write_same_addr_p,harden_p); | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These initial $displays
are a bit noisy, please remove them so when running we only see the reset banners, then the processor outputs.
% git grep -l "instantiating" | cat
src/basejump_stl/bsg_launch_sync_sync.sv
src/basejump_stl/bsg_mem_1r1w.sv
src/basejump_stl/bsg_mem_1r1w_one_hot.sv
src/basejump_stl/bsg_mem_1r1w_sync.sv
src/basejump_stl/bsg_mem_1r1w_sync_synth.sv
src/basejump_stl/bsg_mem_1rw_sync.sv
src/basejump_stl/bsg_mem_1rw_sync_mask_write_bit.sv
src/basejump_stl/bsg_mem_1rw_sync_mask_write_byte.sv
src/basejump_stl/bsg_mem_1rw_sync_synth.sv
src/basejump_stl/bsg_mem_2r1w_sync.sv
src/basejump_stl/bsg_mem_2r1w_sync_synth.sv
src/basejump_stl/bsg_mem_3r1w_sync.sv
src/basejump_stl/bsg_mem_3r1w_sync_synth.sv
src/basejump_stl/bsg_sync_sync.sv
src/basejump_stl/bsg_tag_master.sv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: a few small other prints before the reset banner are fine, e.g. "Initializing mem", let's just remove the many "instantiated" ones to reduce noise.
Also note the 4x4 config seems to fail on dhrystone, but the post check script accepts it: |
This PR serves to import multi-core BlackParrot designs into RTLMeter, with various multi-core workloads.