-
Notifications
You must be signed in to change notification settings - Fork 58
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
Update Verilator to version 5.006 #158
Conversation
I tested this PR in our downstream docker image and testsuite (https://github.com/opencompl/Quidditch/blob/main/runtime/toolchain/Dockerfile) in part to evaluate whether this would then also close #148. Few things sadly happened as soon as I used the newer SHA with the newer verilator release:
|
So the good news is that the new verilator model is a lot faster running a whole NN that leverages all 9 cores of a snitch cluster. Previously it took 5076 seconds; which has now been reduced to 2200 seconds, i.e. more than 50%. I used The correctness issue in #148 seems to have also been fixed |
@zero9178 thanks for the detailed testing and report! The last results look promising, so I would close #148 in favour of this. Did you manage to test this with GCC 11.1 in the end? Or were you able to fix the previous errors? Regarding the log names this should be due to #58, but anyways the traces shouldn't affect the correctness of any of the software tests. |
720c1d7
to
e5f7611
Compare
I sadly haven't gotten around to doing this yet and I am currently short on time, but will do so as soon as possible. |
Looking forward to meeting you and we can surely discuss this in that setting as well :) |
e5f7611
to
3ca593a
Compare
I believe I have found and fixed the issue that affected Clang and potentially the newer GCC versions as well. It seemed to have been a stack overflow after all. Culprit are the files/functions that verilator designates as Slow and therefore compiled with |
Great news! 🎉 I will proceed with the merge then :) |
3ca593a
to
40cff28
Compare
40cff28
to
257229e
Compare
8df70ab
to
54aeece
Compare
2187057
to
2362ebb
Compare
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.
Very nice! thanks a lot. The CI should pass again with #172
Using more threads in the simulation may lead to a speedup in simulation time, especially when the program running in the simulator makes use of multiple cores. This PR therefore adds the `VLT_NUM_THREADS` option to `make` to allow users to set the thread number used in the simulator. For backwards compatibility, and since the number of threads is environment and code dependent, the default value of 1 was kept.
2362ebb
to
3fa7ad7
Compare
3fa7ad7
to
57d1f48
Compare
General
VLT_NUM_THREADS
to configure threads used by verilator #148.Details
--timing
to support timing constructs e.g. as will be present in iDMA tracer.VLT_JOBS
option to control the number of threads, useful to keep memory usage under control.github-ci.hjson
config with a reduced core count (4 cores) for use in the Github CI.default.hjson
, and not withfdiv.hjson
. In any case it's best to align all configurations to the default, so this is included as part of the PR.