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
Add Verilator run-time trace switch #3667
Conversation
959681f
to
94015de
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3667 +/- ##
==========================================
+ Coverage 68.83% 68.98% +0.14%
==========================================
Files 46 45 -1
Lines 8536 8082 -454
Branches 2439 2329 -110
==========================================
- Hits 5876 5575 -301
+ Misses 1518 1395 -123
+ Partials 1142 1112 -30 ☔ View full report in Codecov by Sentry. |
top->trace(tfp.get(), 99); | ||
tfp->open("dump.vcd"); | ||
std::unique_ptr<VerilatedvcdC> tfp(new VerilatedVcdC); | ||
const char* traceFile = "dump.vcd"; |
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.
Any possibility that this can be made into an argument and the FST vs VCD also decided at runtime by looking at the extension?
I'm guessing we could also make VERILATOR_SIM_DEBUG
a runtime flag, no?
Also I'm wondering if there is a reason not to always compile with VM_TRACE
, VM_COVERAGE
and use runtime switches to turn them on. Do they slow down the sim or the build considerably?
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.
Unfortunately the verilated C++ can only be no trace, vcd trace or fst trace. There's no vcd or fst trace option.
An option to set VERILATOR_SIM_DEBUG
while building does sound like a good idea. Seems like a separate commit, but I can add it here is desired.
I have tried benchmarking trace vs no trace models over the years and have never been impressed enough with the performance gains to bother building no trace variants. But other runner flavors seem to support this construct, so maybe it's best to keep it? As I mentioned, we don't plan to use build()
in the Verilator runner so I don't have a strong preference.
Coverage does slow down the Verilator sims so I would not suggest adding that by default.
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.
Unfortunately the verilated C++ can only be no trace, vcd trace or fst trace. There's no vcd or fst trace option.
So Verilator simply doesn't support compiling support for both in and only using the appropriate type? Odd.
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.
Are you not going to put tp->close();
line in a traceOn
guard?
Oh whoops, let me fix that. |
Also add a newsfragment. |
94015de
to
e3bce21
Compare
Adds support for enabling tracing at test-time for Verilator.
This doesn't exactly fit the mold of the runner class, but there is also not a consistent story across the various simulators right now. Currently some simulators (e.g. Icarus) will only compile in trace support if
waves
isTrue
, while others (e.g. Xcelium) always compile in trace support and only observewaves
at test time.If the status quo is fine, I'm happy to leave things as-is. However, if it's desirable to split
waves
intobuild_for_waves
andwaves
(or whatever) I can add that here as well.This is useful to us (and probably others) since monolithic Verilator builds are painfully slow once you reach a certain point. I don't believe solving that problem is in scope for the cocotb runner, so we (and possibly others) will be using
test()
but notbuild()
for Verilator.Related: Wilson has some good notes on Verilator building here:
https://veripool.org/papers/Verilator_Accelerated_OSDA2020.pdf