Skip to content
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 --disable-tracing and --prefix-trace options to snax_cluster #399

Merged
merged 9 commits into from
Nov 27, 2024

Conversation

JosseVanDelm
Copy link

@JosseVanDelm JosseVanDelm commented Nov 22, 2024

This adds two extra options to snitch_cluster, to address #395

This was quite difficult to figure out, the way i've implemented it now is like this:

2 C++ functions that return a command-line value --> DPI-C --> SystemVerilog Tracer

  • --disable-tracing : turns off tracing for the Verilog simulation, might speed up some long-taking CI runs, for which we don't care about log parsing anyway...
  • --prefix-trace="prefix": appends a prefix to the output traces. If you use this, it will not default to a logs folder anymore, so be sure to include logs/your_prefix in the prefix if you want it to end up in a folder.

I'm keeping this as a draft for now because of the following reasons:

  • I don't know how to test this for VCS or Questa (@IveanEx @xiaoling-yi ) can you help me with that? Right now it is implemented in a way that is quite likely to break those latter two, although it should be easily fixable with some preprocessor magic (that would make it impossible to use this functionality on VCS or questa though...)
  • The argument parser is really janky... It separately parses the options I added, and then passes this on to the simulator. The reason for this is that the rest of the argument parsing is done by htif.cc from riscv-isa-sim/fesvr... Not sure if we are using any of those arguments right now though...
  • @IveanEx has pointed out that there are multiple other places in the systemverilog that write to a separate log file. I would like to detect those and make them happen in a single place... the current way is pretty ugly/impractical...

hw/snitch_cluster/src/snitch_cc.sv Outdated Show resolved Hide resolved
if(enable_tracing()) begin
$sformat(fn, "trace_chip_%01x%01x_hart_%05x.dasm", tcdm_addr_base_i[47:44],
tcdm_addr_base_i[43:40], hart_id_i);
$sformat(trace_file, "%s%s", get_trace_file_prefix(), fn);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to concatenate the strings here in systemverilog, but that made the verilator quite angry, so now I'm using this really dumb sformat

return default_value; // Return the default value if not found or invalid
}

svBit enable_tracing() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to svBit, because verilator seemed to generate a DPI function that expected that... not sure how else I would've figured out...

target/common/test/verilator_lib.cc Outdated Show resolved Hide resolved
@JosseVanDelm JosseVanDelm force-pushed the Josse/add-prefix-functionality branch 3 times, most recently from c6d2bfd to c636b56 Compare November 25, 2024 15:03
@JosseVanDelm
Copy link
Author

JosseVanDelm commented Nov 25, 2024

@IveanEx has pointed out that there are multiple other places in the systemverilog that write to a separate log file. I would like to detect those and make them happen in a single place... the current way is pretty ugly/impractical...

@IveanEx the only other place I have found this was already behind an ifndef VERILATOR inside hw/future/src/dma/axi_dma_backend.sv, so I'd mark this problem as solved

@JosseVanDelm JosseVanDelm marked this pull request as ready for review November 25, 2024 15:07
@JosseVanDelm JosseVanDelm force-pushed the Josse/add-prefix-functionality branch from c636b56 to ba027aa Compare November 25, 2024 15:08
@JosseVanDelm JosseVanDelm force-pushed the Josse/add-prefix-functionality branch from ba027aa to 31344b9 Compare November 25, 2024 15:12
Copy link

@IveanEx IveanEx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR breaks the HeMAiA's Verilator simulation flow (make hemaia_system_vlt at the root directory). Can you also make a PR to the main branch of HeMAiA to fix it?

Beside this error, other tests with Questa and VCS can pass without problem.

@JosseVanDelm JosseVanDelm changed the title Add tracer disable/prefix functionality Add --disable-tracing and --prefix-trace options to snax_cluster Nov 26, 2024
@JosseVanDelm JosseVanDelm requested a review from IveanEx November 26, 2024 14:52
@JosseVanDelm
Copy link
Author

This PR breaks the HeMAiA's Verilator simulation flow (make hemaia_system_vlt at the root directory). Can you also make a PR to the main branch of HeMAiA to fix it?

@IveanEx
PR is ready at: KULeuven-MICAS/HeMAiA#90

@JosseVanDelm JosseVanDelm merged commit 5b1111a into main Nov 27, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants