target: Add VLT_NUM_THREADS to configure threads used by verilator
#148
Conversation
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.
zero9178
changed the title
VLT_NUM_THREADS to threads used by verilatorVLT_NUM_THREADS to configure threads used by verilator
There was a problem hiding this comment.
Thanks for the valuable contribution @zero9178!
I have just a few comments to simplify the change further. Let me know if you think it could work.
Also, I would perhaps update the CI to use the multi-threaded version, even with just two threads, to verify the implementation.
target/common/common.mk
Outdated
| ifneq ($(VLT_NUM_THREADS), 1) | ||
| VLT_FLAGS += --threads $(VLT_NUM_THREADS) | ||
| endif |
There was a problem hiding this comment.
I believe we can drop the if statement, as Verilator should by default behave as if --threads 1 is passed.
There was a problem hiding this comment.
I believe this is true for Verilator 5 but not the verilator 4 that the repo currently supports until #76 lands.
That said I think you're right and we can drop this as this is almost certainly a premature optimization and I doubt single-threaded usage will see a measureable difference.
target/common/common.mk
Outdated
| ifneq ($(VLT_NUM_THREADS), 1) | ||
| VLT_CFLAGS += -DVL_THREADED=1 | ||
| endif |
There was a problem hiding this comment.
Couldn't find much information on this, but perhaps we can also always add this flag, and remove the if statement. I guess it shouldn't break the single-threaded case.
There was a problem hiding this comment.
Information is sadly very sparse on the flag and I only found it referenced through GitHub issues such as verilator/verilator#3668 (comment)
I had to add it as I was getting compilation errors as verilator requires it to be defined when using multiple threads.
I added a comment to clarify.
|
I am seeing issues with the simulation where it seemingly gets stuck/deadlocks for non-obvious reasons. While the PR itself is okay its probably not a good idea to actually use any value but 1 for the time being. |
See also issue #116. @and-ivanov may know something more about this. Tagging him in case he might have further comments. |
|
Dear @zero9178, I opened a new PR upgrading the supported Verilator version to 5.006 (#158). I tentatively added your multi-threading contributions from this PR. If you would like to try it, perhaps it fixes the issues you mentioned above. If this is the case we can close this PR in favour of #158 and merge it. Best, |
That's amazing! Let me run the verilator model from your PR through our stack and I'll get back to you whether it works (I'll respond on that PR). If yes then I think we can close this. If the multithreading part does not work then I'd either leave this open and/or make this an issue. |
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_THREADSoption tomaketo 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.