-
Notifications
You must be signed in to change notification settings - Fork 651
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
Multithreaded Verilator #654
Conversation
This is untested. Additionally, someone may want to double-check the |
@@ -165,15 +169,15 @@ verilog: $(sim_vsrcs) | |||
|
|||
# run normal binary with hardware-logged insn dissassembly | |||
run-binary: $(output_dir) $(sim) | |||
(set -o pipefail && $(sim) $(PERMISSIVE_ON) $(SIM_FLAGS) $(EXTRA_SIM_FLAGS) $(SEED_FLAG) $(VERBOSE_FLAGS) $(PERMISSIVE_OFF) $(BINARY) </dev/null 2> >(spike-dasm > $(sim_out_name).out) | tee $(sim_out_name).log) | |||
(set -o pipefail && $(NUMA_PREFIX) $(sim) $(PERMISSIVE_ON) $(SIM_FLAGS) $(EXTRA_SIM_FLAGS) $(SEED_FLAG) $(VERBOSE_FLAGS) $(PERMISSIVE_OFF) $(BINARY) </dev/null 2> >(spike-dasm > $(sim_out_name).out) | tee $(sim_out_name).log) |
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.
What happens when you set this but use vcs?
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.
From testing on a5
, nothing since the script returns nothing. In general, I don't think you want to enable NUMA_PREFIX
, when using VCS though.
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.
Did you do any tests where the script does return something?
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.
No I didn't. It shouldn't really be used with VCS but even if it was I think numactl
is fairly understandable, it just controls where the program is placed (what CPU/Socket) and nothing more.
|
||
// RocketChip currently only supports RBB port 0, so this needs to stay here |
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.
The new <widget>_t
should be set by the simulator widget code when first ticked. The main difference between this and the orig. PR is that you need to have the jtag
widget struct initialized again otherwise the rbb_port
will not be assigned to it.
Ok this was tested with the |
9162279
to
2168813
Compare
Can you verify that |
I can confirm that |
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 doing this for me! Much easier for me to understand whats going on.
I now wonder if the script should use the number of threads requested in some way but I don't think its very important now.
scripts/numa_prefix
Outdated
# loop through available nodes, selecting the node with the most free mem | ||
for i in avail_nodes: | ||
cpu_line = lines[line_idx] | ||
mem_size_line = lines[line_idx + 1] |
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.
This isn't used anywhere. I think we should just remove it and comment on mem_free_line that we skip the size line.
Related issue: Split up from PR #562
Type of change: new feature
Impact: other
Release Notes
This splits up #562 to just include the multi-threaded Verilator improvements. The main differences from the prior PR is that
numa_prefix
is moved toscripts/
,numa_prefix
is directly pointed to incommon.mk
, andNUMA_PREFIX
is added in more places (only where Verilator might be called).Finally, I caught extra flags being passed to
LDFLAGS
that were unneeded and used theTRACE_OPTS
that was missed in PR #650 .