Added a workaround for parameter-controlled elaboration in Verilator

[drichmond]

This seems like a simple fix, but I want to make sure we're all on board because it has some subtle (and annoying) implications.

There are two changes. First, because Verilator doesn't correctly handle parameter-controlled instantiation of modules, our methodology for disabling/enabling the profilers doesn't work in Verilator. Instead, we need to "clock gate" them like so:

bind vanilla_core vanilla_core_profiler #(
    .x_cord_width_p(x_cord_width_p)
    ,.y_cord_width_p(y_cord_width_p)
    ,.icache_tag_width_p(icache_tag_width_p)
    ,.icache_entries_p(icache_entries_p)
    ,.data_width_p(data_width_p)
    ,.origin_x_cord_p(`BSG_MACHINE_ORIGIN_X_CORD)
    ,.origin_y_cord_p(`BSG_MACHINE_ORIGIN_Y_CORD)
  ) vcore_prof (
    .*
    ,.clk_i(clk_i && $root.`HOST_MODULE_PATH.testbench.enable_vcore_profiling_p)

I added a comment explaining this. Also, I used && because I'm worried that someone (or more likely some tool) will use some value that isn't strictly 1 (e.g. "yes"). Using && gets around this.

The second change is the instance name of bsg_nonsynth_manycore_testbench in spmd_testbench.v. I switched it from tb, to testbench because we want to reference the parameters (above) from bsg_nonsynth_manycore_testbench.v, not the top level.

I realize this might be the most controversial change and I agree it's annoying. We typically use this instantiation path in other tools (e.g. saif analysis). I changed the SAIF header string in the path to reflect this but I realize that there are other scripts that may depend on tb, not testbench. In replicant, we actually depend on testbench, and my impression was that there was more instances to change there than in manycore (hopefully just the one above?)

This seemed like the shortest path, but I'm willing to be convinced that it will break some other part of the flow and find a better way to handle it.

[drichmond]

@tommydcjung I forgot to add ci_ to this. How do I make it run without changing the branch? I've forgotten the steps

[tommydcjung]

Is this bug going to be fixed in verilator? I see that the benefits of verilator compatibility could outweight the minor and temporary sacrifice in code quality, but this seems like a simple bug that can be fixed in verilator that can benefit greater open source community in general.

[tommydcjung]

Approved, but I'm going to leave the ultimate burden to decide if it's okay on Michael

[drichmond]

The answer is I don't know. Both Dan and I thought there was an issue filed in Verilator, but we can't find it.

[dpetrisko]

The answer is I don't know. Both Dan and I thought there was an issue filed in Verilator, but we can't find it.

We should file a new one, but latency will be weeks at earliest. So I would suggest to commit the workaround in the meantime

[drichmond]

This is the relevant Verilator issue: https://github.com/verilator/verilator/issues/1501

I've filed an issue in manycore, Dan has posted it there, and I'm now subscribed to any updates on the issue.

I am merging this for now, but will undo/redo when I get a notification that it is fixed.

@tommydcjung Is that sufficient?

[tommydcjung]

This issue was created 3 years ago, and hasn't made any progress since then... It was probably de-prioritized because Dan said it's not a show stopper..

[dpetrisko]

And indeed it wasn’t

[tommydcjung]

Besides I think the measures taken are sufficient, but I would just give them a friendly reminder about the issue on verilator-side.

[drichmond]

OK, I switched to macros that are specific to verilator for the time being. VCS behavior remains unchanged. @tommydcjung OK with you?

[tommydcjung]

OK, I switched to macros that are specific to verilator for the time being. VCS behavior remains unchanged. @tommydcjung OK with you?

OK with me. thanks