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

feat: add waveform tracing support for simulateRaw and simulate in SimulatorAPI #4770

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Emin017
Copy link
Contributor

@Emin017 Emin017 commented Mar 5, 2025

This commit introduces a new enableTrace parameter to the simulate and simulateRaw methods in SimulatorAPI.scala, allowing users to enable waveform tracing during simulations. Additionally, corresponding test cases have been added to ChiselSimSpec.scala to verify that waveform files are correctly generated for both Module and RawModule simulations.

By adding this feature, users can enable tracing like this:

class GCDSpec extends AnyFreeSpec with Matchers {
  class Foo extends Module {
    val a = IO(Input(Bool()))
    val b = IO(Output(Bool()))

    b :<= !a
  }
  implicit val verilator = simulators
    .verilator(verilatorSettings =
      svsim.verilator.Backend.CompilationSettings(
        traceStyle =
          Some(svsim.verilator.Backend.CompilationSettings.TraceStyle.Vcd(traceUnderscore = true, "trace.vcd"))
      )
    )
  simulate(new Foo, enableTrace = true) { foo =>
    foo.a.poke(true.B)
    foo.b.expect(false.B)
    foo.a.poke(false.B)
    foo.b.expect(true.B)
  }(hasSimulator = verilator)
}

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Feature (or new API)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

…mulations

This commit introduces a new `enableTrace` parameter to the `simulate`
and `simulateRaw` methods in `SimulatorAPI.scala`, allowing users to
enable waveform tracing during simulations. Additionally, corresponding
test cases have been added to `ChiselSimSpec.scala` to verify that
waveform files are correctly generated for both `Module` and `RawModule`
simulations.

Signed-off-by: Qiming Chu <[email protected]>
@sequencer
Copy link
Member

sequencer commented Mar 5, 2025

I think it should be abstract to an API with SimulatorConfig or something like this, there are many other things to configure the simulator and different simulator has different behaviors, for example fst, vcd choice for verilator, fsdb for vcs, threading configs…
If adding a new feature please consider what’s other options needed to be added in the future.

@Emin017
Copy link
Contributor Author

Emin017 commented Mar 5, 2025

I think it should be abstract to an API with SimulatorConfig or something like this, there are many other things to configure the simulator and different simulator has different behaviors, for example fst, vcd choice for verilator, fsdb for vcs, threading configs… If adding a new feature please consider what’s other options needed to be added in the future.

I appreciate your concern about API design and future extensibility. Here's my reasoning:

  1. The current enableTrace parameter addresses a common user need with minimal API surface change
  2. The implementation delegates details to backends via controller.setTraceEnabled() without prescribing how tracing happens
  3. Backend-specific configurations (like trace format selection or threading configs) can still be handled through the existing HasSimulator typeclass
  4. This approach doesn't preclude future adoption of a SimulatorConfig object

This design maintains API simplicity for the common case while preserving extension points for backend-specific configuration.

Would you prefer we implement the comprehensive SimulatorConfig in the future, or is this incremental improvement acceptable with understanding we can refactor later as additional configuration needs arise?

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Thanks for starting on this. It is really annoying in that it is basically impossible to turn on waveform dumping today.

Couple of things:

  1. I agree with Jiuyang and there's currently a bunch of options that need to be handled for waveforms w/ different simulators handling things differently. However, this may be fine as is as a top level "turn on waveform dumping option" (like the PR adds) and then leave the simulator-specific waveform-enabling options to backends. The one thing that needs to be validated is that these are properly synchronized. If a user turns on waveforms, a proper backend option should be set automatically.
  2. The trace enabling from a specific point in time is actually useful. Given this, I'm not sure if we actually want to turn on waveform dumping as an option and instead want to do it via stimulus. Going in this direction, this would then mean adding something to the stimulus package (like RunUntilFinished that just turns on waveforms). That would require figuring out how to make it work with the initialization procedure of the simulate method as you would like to have a wave for the whole simulation and not just post-reset.

@sequencer: Do you have any thoughts about this?

Comment on lines -27 to +28
chiselSettings: ChiselSettings[T] = ChiselSettings.defaultRaw[T]
chiselSettings: ChiselSettings[T] = ChiselSettings.defaultRaw[T],
enableTrace: Boolean = false
Copy link
Member

Choose a reason for hiding this comment

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

Generally, I think we do want to expose this. However, it would be better to make it part of ChiselSettings. I'd like to keep the options organized under ChiselSettings and not have a proliferation of other options.

@seldridge
Copy link
Member

There are already per-backend options for compiling different backends with support for different trace formats:

and
final case class TraceSettings(
. Note: the software architecture of the backends is pretty weird and I haven't gotten to cleaning them up, yet...

@seldridge seldridge added the Feature New feature, will be included in release notes label Mar 5, 2025
@seldridge
Copy link
Member

seldridge commented Mar 5, 2025

I thought about it a bit more. What about this:

  1. Add an option to ChiselSettings called enableWavesAtTimeZero: Boolean. A better name is fine.
  2. Benchmark changing the behavior so that VCD dumping support is turned on by default for all backends. This should then be changeable for VCS to VPD or FSDB. (I would like to make some waveform dumping always possible as then we don't have the footgun of the user turning on waveforms, but they haven't compiled in support. The question here is is there any cost to $dumpfile and $dumpvars(0, <dut>) for a massive design.) If we can't always compile with waveform dumping support, then we would need to do a runtime check that the user isn't trying to dump waves when support doesn't exist. This could be handled by an svsim response to the dump waves command indicating "no support".
  3. Add new stimulus to the stimulus package called EnableTrace and DisableTrace (or one option that takes a boolean) which then allows users to actually turn this on. (I don't think users can do this today as you need access to a controller and that is not possible to get.)

WDYT?

Also (and again), @sequencer WDYT?

@Emin017
Copy link
Contributor Author

Emin017 commented Mar 6, 2025

  1. Add an option to ChiselSettings called enableWavesAtTimeZero: Boolean. A better name is fine.

Agree with this approach. I think adding another parameter feels a bit cluttered, too.

  1. Benchmark changing the behavior so that VCD dumping support is turned on by default for all backends. This should then be changeable for VCS to VPD or FSDB. (I would like to make some waveform dumping always possible as then we don't have the footgun of the user turning on waveforms, but they haven't compiled in support. The question here is is there any cost to $dumpfile and $dumpvars(0, <dut>) for a massive design.) If we can't always compile with waveform dumping support, then we would need to do a runtime check that the user isn't trying to dump waves when support doesn't exist. This could be handled by an svsim response to the dump waves command indicating "no support".

Regarding enabling waveform dumping by default for all backends, my concern is: would this impact simulation performance or other aspects? However, I do think including waveform saving options during compilation by default would help implement the "start saving at specific points" functionality you mentioned.

  1. Add new stimulus to the stimulus package called EnableTrace and DisableTrace (or one option that takes a boolean) which then allows users to actually turn this on. (I don't think users can do this today as you need access to a controller and that is not possible to get.)

I had considered accessing the controller externally in serveral other ways, but didn't get the desired results, which is why I used the current approach.

@seldridge
Copy link
Member

Regarding enabling waveform dumping by default for all backends, my concern is: would this impact simulation performance or other aspects? However, I do think including waveform saving options during compilation by default would help implement the "start saving at specific points" functionality you mentioned.

Right, I want to know if this is a performance problem. Note: I'm only saying to enable dumping, not to actually dump. I.e., the default would be for VCD:

$dumpfile("foo.vcd")
$dumpvars(0, foo)

Is this slow even if $dumpon is never called?

The code for how this works in Chiselsim is here: https://github.com/chipsalliance/chisel/blob/main/svsim/src/main/scala/Workspace.scala#L169 You'll see that VCD, VPD, and FSDB are all gated by macros which is what those backend options are controlling.

@Emin017 Emin017 marked this pull request as draft March 6, 2025 15:31
@seldridge
Copy link
Member

seldridge commented Mar 7, 2025

There is another option that could be explored here. Command line options can be passed using Scalatest's config map. I've added support for this in the HasConfigMap trait which has a user with the HasCliSimulator trait. E.g., if you use this, you can then select the simulator via -Dsimulator=verilator or -Dsimulator=vcs.

Internally, we have historically had an option -DwriteVcd=1 which tests supported. This is the other direction that we could go here and say that waveform generation is something you do usually do the command line as opposed to something that you do via a ChiselSetting. We also build with waveform support always, just don't turn it on.

seldridge added a commit that referenced this pull request Mar 7, 2025
Add Scalatest integration for Chiselsim which allows users to write traits
which add command line options to a Scalatest test.  These hook into the
`-D` Scalatest options (i.e., the ConfigMap) and allow the user to modify
the common or backend-specific options of a test.  This includes
validation features to guarantee that illegal options are not passed.

Add an example trait, `WaveformVCD` which will enable VCD dumping for
_either_ Verilator or VCS.  With this, you can then do, for a test which
adds this trait:

    ./mill 'chisel3[2.13.16].test.testOnly' 'FooSpec' -- -DemitVCD=true

This is related to #4770.

Signed-off-by: Schuyler Eldridge <[email protected]>
@sequencer
Copy link
Member

Also (and again), @sequencer WDYT?

sorry I missed this.

We also build with waveform support always, just don't turn it on.

There is almost no overhead in both compile time and runtime from my experience in vcs. But introducing compile time issue in verilator.

Is this slow even if $dumpon is never called?

Almost no runtime overhead from my understanding.

I think the API design of this PR is fine if Schuyler approves;)

From my experience, I’m a little bit concerned about adding more simulation features, we may eventually go into the Verilog Verbatim nightmare like t1 https://github.com/chipsalliance/t1/blob/master/t1rocketemu/vsrc/VerbatimModule.sv

@sequencer
Copy link
Member

Essentially we need to think about some fundamental issues when adding new APIs:

  • what’s the purpose of ChiselSim?

  • Fully replace the UVM-like simulation?

  • Being a UnitTest generator for small Modules in the generator?

  • Just a smoke test for most of modules.

  • What backend does it support?

  • verilator + vcs/ncsim?

  • arc?

  • what’s the suggested flow for ChiselSim?

  • how does it integrate with mill/sbt build system?

  • How does it use from command lines?

@seldridge
Copy link
Member

I thought about this even more. Here is how I would like to proceed:

  1. We add a missing ControlAPI to ChiselSim. This should include the ability to turn waves on and off: [chiselsim] Add ControlAPI w/ Waveform Enable/Disable Support #4779
  2. Change this PR to not modify simulateRaw and to add an argument to simulate called enableWavesAtTimeZero. ChiselSettings has no mechanism to turn waves on/off.
  3. ChiselSettings gains an argument called waveformSupport: Seq[WaveFormStyles] which will add the appropriate option for different waveform styles to compilation.
  4. We add the ability to update compile options on the command line: [chiselsim] Add ControlAPI w/ Waveform Enable/Disable Support #4779

I realize that (3) and (4), along with the existing backend-specific compilation options means that there are three redundant ways to compile in waveform support. However, the backend-specific options are what we are trying to not have users use and (3) and (4) are two different ways of using ChiselSim. One is pure ChiselSim. The other is more Scalatest friendly.

@Emin017
Copy link
Contributor Author

Emin017 commented Mar 8, 2025

  1. We add a missing ControlAPI to ChiselSim. This should include the ability to turn waves on and off: [chiselsim] Add ControlAPI w/ Waveform Enable/Disable Support #4779

I agree with this approach of adding a ControlAPI to ChiselSim. This would indeed provide more direct and flexible control over waveform generation compared to just adding options in ChiselSettings (We can get access to the controller more directly).

I realize that (3) and (4), along with the existing backend-specific compilation options means that there are three redundant ways to compile in waveform support. However, the backend-specific options are what we are trying to not have users use and (3) and (4) are two different ways of using ChiselSim. One is pure ChiselSim. The other is more Scalatest friendly.

I think the backend-agnostic abstraction for ChiselSim is a nice design that helps provide a more user-friendly API. But I think we could also allow users to pass their compile parameters to backends like verilator, vcs or arcilator (maybe in the future?). This would help in a couple of ways:

  1. Reduce future complexity as new simulator features are added. By providing this flexibility, we can avoid requiring API changes for each new backend option.
  2. Support advanced users who need fine-grained control

BTW, I'd like to ask if ChiselSim currently has any plans to support the arcilator backend ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants