Introduce `svsim`, a low level library for simulating SystemVerilog using Verilator and VCS. #3121

GeorgeLyon · 2023-03-20T22:52:39Z

I apologize in advance to not-SiFive folks for ramboing a 2K+ line PR (the SiFive folks have had a bit more of a gradual introduction to this). You can be the final judge, but I think it is quite clean an readable and I will be covering some of the implementation details at the next Chisel Dev meeting (3/27) if folks are interested or have questions that don't work well in a PR context.

Type of Improvement

This upstreams the svsim project, which is a simple, self-contained Scala library for compiling SystemVerilog simulations using Verilator and VCS, and then executing and controlling those simulations. We built svsim at SiFive to enable simulating MFC-emitted SystemVerilog in the absence of SFC. svsim is meant to be low-level and not used directly by hardware designers. As such, its documentation is mainly in the code and in svsim/README.md.

A subsequent PR will migrate the BasicTester infrastructure to use svsim, and clean up that API so that users of Chisel have a built-in mechanism for running simple tests. We can then document how to use chisel3.testers to enable simple unit testing of Chisel modules.

Relation to `chiseltest`

While svsim may draw comparisons to chiseltest, I think it is more accurate to consider svsim as a lower-level component that could be used to power chiseltest's simulation capabilities (svsim is specifically not a testing framework). The "Design" section of svsim/README.md discusses some of the things chiseltest does which are outside of the scope of svsim.

API Impact

This does not impact API.

Backend Code Generation Impact

This does not impact backend code generation.

Desired Merge Strategy

Squash

Release Notes

Added svsim, a new library for compiling and controlling SystemVerilog simulations in Scala using Verilator or VCS.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.5.x or 3.6.x depending on impact, API modification or big change: 5.0.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), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

GeorgeLyon · 2023-03-20T22:54:44Z

.devcontainer/Dockerfile

@@ -2,6 +2,14 @@ FROM --platform=$BUILDPLATFORM ubuntu:20.04

 ARG TARGETARCH

+# Initialize System


These changes are PR-ed separately here: #3120. I'm expecting that PR to merge soon and then these will be removed from this PR.

GeorgeLyon · 2023-03-20T22:56:15Z

build.sbt

-          } else {
-            "-Werror" :: Nil
-          }
+        if (sys.props.contains("disableFatalWarnings")) {


It seems like build.sbt was not formatted with the other sources, so VSCode decided to format it for me. I can undo these changes if folks care but I don't think it is a bad thing to have this file formatted.

ekiwi · 2023-03-20T23:10:00Z

svsim/README.md

+* `svsim` simulations are also backend-independent, meaning that the same code launches and controls a simulation regardless of what backend it was compiled using. 
+* `svsim` backends are declarative. This means a backend only controls which executable is invoked for compilation, and what arguments are provided to that executable (either via command line or environment variables). Backends may also provide arguments to the simulation, but may not have any other specialized logic.
+* Simulations are controlled using an ad-hoc protocol where commands are read from `stdin` and messages are written to `stdout`. This protocol is **not** meant to be an ABI, and can change freely between Chisel verisons.
+* Communication with the simulation is pipelined, so the Scala driver can send multiple commands before processing responses from the simulation. This allows `svsim` to effectively eliminate the overhead of communicating with the simulation without resorting to JNA or Scala-side caching. This optimization can, of course, be defeated if the driver code waits on every command, but we consider this an antipattern.


I am curious: did you benchmark this? Is it actually as fast as calling into a C-library function directly?

I like to say the performance is "analogous". Across SiFive's test suite, svsim performed a bit faster than chiseltest (though I think the speedup was mostly due to not running any SFC at all, also storing all this stuff in maps for VCS is easy to outperform). There were some tests that slowed down a little, and these were tests which were repeatedly waiting on the value of a peek (and not just using expect, which pipelines nicely). Obviously a straight-up C function call is faster, but compiling and simulating even a small design dwarfs this performance cost pretty quickly. For many tests, there are less than 10 "commands" sent to the simulation (svsim implements TICK as a single command) so this overhead is negligible if you are not blocking on the IO.

Thanks. That makes sense. So I guess most of the SiFive tests are BasicTester style where they only wait for a stop or a failed assert?

There are peek poke tests as well. As long as they don't have a data dependency (i.e. they only use expect and not peek) the performance is analogous (and maybe faster if it is VCS due to the map issue above)

GeorgeLyon · 2023-03-20T23:32:50Z

@jackkoenig This test failure is confusing, could you provide any insight? https://github.com/chipsalliance/chisel/actions/runs/4473691082/jobs/7861373685?pr=3121#step:7:2576

It doesn't seem related to anything I changed, but I don't see anything that would cause it to flake and that Seq looks to very much not be of size 5? https://github.com/GeorgeLyon/chisel/blob/cc41b964b26bbbaa07e8c7b3655370c6b7d4caa7/src/test/scala/chiselTests/Vec.scala#L470-L478

mwachs5 · 2023-03-20T23:54:13Z

.devcontainer/Dockerfile

+      --target install-firtool
+
+# Use VSCode for writing commit messages
+ENV GIT_EDITOR="code --wait"


is this necessary for this PR?

No, these are broken out in a separate PR I'm hoping to merge soon: #3120

mwachs5 · 2023-03-20T23:54:43Z

.devcontainer/Dockerfile

@@ -25,8 +33,9 @@ RUN \
 # Install GraalVM
 # This downloads all relevant GraalVM architectures at once, mostly because $TARGETARCH values don't map exactly to the release URLs. Since we're optimizing for developer experience here and not image size, this is OK
 # GraalVM release links can be found here: https://github.com/graalvm/graalvm-ce-builds/releases
-ADD https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-22.3.1/graalvm-ce-java17-linux-aarch64-22.3.1.tar.gz /graalvm/tarballs/arm64.tar.gz
-ADD https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-22.3.1/graalvm-ce-java17-linux-amd64-22.3.1.tar.gz /graalvm/tarballs/amd64.tar.gz
+ENV JAVA_VERSION=java11


why are we going from java17 to java11?

Java17 fails to compile what remains of FIRRTL. I didn't dig into this too deeply. Also worth calling out this is just for development. None of these are affecting the overall requirements of the Chisel project.

.devcontainer/Dockerfile

mwachs5 · 2023-03-20T23:57:04Z

build.sbt

@@ -334,98 +347,101 @@ lazy val chisel = (project in file("."))
  .dependsOn(macros)
  .dependsOn(core)
  .dependsOn(firrtl)
-  .aggregate(macros, core, plugin, firrtl)
+  .dependsOn(svsim)


why does chisel the project depend on svsim? Shouldn't this go in the chisel test project dependencies, if anywhere today?

(also I don't really understand how this file works, so my question may be nonsensical)

I'm looking at chisel.testers to host the "glue svsim to Chisel code". This is part of the chisel project AFAICT, but I may be missing something about test vs non-test dependencies.

mwachs5 · 2023-03-20T23:58:55Z

build.sbt

+  .settings(warningSuppression: _*)
+  .settings(fatalWarningsSettings: _*)
+  .settings(chiselSettings: _*)
+  .settings(usePluginSettings: _*)


if we are going to format this file in this PR, can we make sure that the process is repeatable. What settings was vscode picking up? I guess I'd strongly prefer we make the "format the build.sbt file and make sure it stays formatted" in its own PR

I believe these are the settings from the project. I would guess the issue is this file doesn't have a ".scala" file extension so its not picked up in CI?

I agree we should make this repeatable, looking in to how we can apply it to the build.sbt

There is a command scalafmtSbt which will format *.sbt files and project/*.scala files. We should see if we can make this part of "all". I will say, we should do this as a separate PR rather than as part of this one just to keep the history more clear.

I've created a PR to format the SBT files: #3125

Thanks, @jackkoenig!

mwachs5 · 2023-03-21T00:00:52Z

svsim/README.md

+
+### Workspace
+
+The `Workspace` manages all interaction with the filesystem. Its API is mutable, because the underlying representation (files on disk) is mutable. A `Workspace` is represented on the filesystem as a folder. The `reset()` method will delete any previous state and create the necessary folders. The `elaborate()` method takes a `ModuleInfo` which describes the SystemVerilog module to be simulated. The idea is that a higher-level framework will provide specialized `elaborate` methods which emit SystemVerilog to `primarySourcesPath`, similar to what we do with `elaborateGCD()` in `src/test/scala/Resources.scala`. For example, Chisel can provide an `elaborate[T <: RawModule](module: => T)` method. `generateAdditionalSources()` uses the `ModuleInfo` from `elaborate` and generates the test harness. Finally, `compile()` uses a `Backend` to compile the simulation. Because `svsim`'s test harness is backend-independent, `compile` may be called multiple times with different settings or different backends to create multiple `Simulation`s (a `workingDirectoryTag` is provided so that mutliple simulations are output to separate directories). 


nit can we wrap this file either at 80 characters or at punctuation

Is a Workspace thread-safe or should it only ever be called from a single thread?

Is there a lint/format pass we can do to add this? My IDE just auto-wraps, but I can also do this manually.

Is a Workspace thread-safe or should it only ever be called from a single thread?

It is not explicitly thread safe, though you could theoretically run multiple compile invocations on multiple threads. Not sure why you would want to do that though. Simulation could be made thread-safe but currently the simulation logs would bork each other. I'd be interested in exploring this more if we ended up with a concrete use case that we want to support.

i think we just have to tell the .scalafmt.conf to target files with sbt extension as well

i think we just have to tell the .scalafmt.conf to target files with sbt extension as well

This is an md file, and I don't see anything in the conf related to file extensions. I also don't see an option to do this in VSCode (independent of scalafmt)... do folks just manually add returns? Seems onerous...

Oh sorry didn't realize your question was for this thread.

I have my vscode to set the .md width to 80

mwachs5 · 2023-03-21T00:02:41Z

svsim/README.md

+
+### `make replay`
+
+`svsim` by default emits an `execution-script.txt` when running a Simulation. This script captures all commands sent to the simulation, which enables `svsim` to add a second target to the `Makefile`: `make replay`. `make replay` rebuilds the simulation, picking up any changes as mentioned above, and then replays the commands that were sent by `Simulation.Controller` verbatim. This allows replaying most tests without having to re-run the Scala code, potentially with manual modifications to either the SystemVerilog or the test harness (though tests which modify their behavior based on values read from the `Simulation` may not be replayed faithfully).


should we document the format of this replay file here? If not here, where is it documented? I see that it is not an ABI but it is a thing that if you're telling people they can edit we should at least point them to what the current format is.

I want to specifically leave it un-documented (though simulation-driver.c has some pretty explicit documentation) mostly because of the "This is not an ABI" point above, and folks shouldn't be using it across Chisel versions.

jackkoenig · 2023-03-21T01:14:57Z

@jackkoenig This test failure is confusing, could you provide any insight? https://github.com/chipsalliance/chisel/actions/runs/4473691082/jobs/7861373685?pr=3121#step:7:2576

What looks like the failure isn't really the failure, the red there is coming from Chisel but that's an expected failure. If you scroll up on the logic you can see other similar examples. We really ought to fix Chisel's error reporting so that it doesn't show up here 😬

The real failure is in compilation of svsim above:

[info] Setting Scala version to 2.12.17 on 10 projects.
[info] Reapplying settings...
[info] set current project to chisel (in build file:/home/runner/work/chisel/chisel/)
[info] compiling 4 Scala sources to /home/runner/work/chisel/chisel/plugin/target/scala-2.12.17/classes ...
[info] Non-compiled module 'compiler-bridge_2.12' for Scala 2.12.17. Compiling...
[info] compiling 6 Scala sources to /home/runner/work/chisel/chisel/macros/target/scala-2.12/classes ...
[info]   Compilation completed in 10.646s.
[info] done compiling
[info] compiling 5 Scala sources to /home/runner/work/chisel/chisel/svsim/target/scala-2.12/classes ...
[info] done compiling
[info] compiling 60 Scala sources to /home/runner/work/chisel/chisel/firrtl/target/scala-2.12/classes ...
[info] done compiling
[info] compiling 2 Scala sources to /home/runner/work/chisel/chisel/svsim/target/scala-2.12/test-classes ...
[error] /home/runner/work/chisel/chisel/svsim/src/test/scala/BackendSpec.scala:74:23: Invalid literal number
[error]           val aVal = -0x0240_0000_0000_0000L
[error]                       ^
[error] /home/runner/work/chisel/chisel/svsim/src/test/scala/BackendSpec.scala:75:23: Invalid literal number
[error]           val bVal = +0x0180_0000_0000_0000L
[error]                       ^
[error] /home/runner/work/chisel/chisel/svsim/src/test/scala/BackendSpec.scala:81:39: Invalid literal number
[error]             assert(value.asBigInt === 0x7dc0_0000_0000_0000L)
[error]                                       ^
[error] /home/runner/work/chisel/chisel/svsim/src/test/scala/BackendSpec.scala:113:39: Invalid literal number
[error]             assert(value.asBigInt === 0x00c0_0000_0000_0000L)
[error]                                       ^
[error] four errors found

Allowing _ in literals was only added in Scala 2.13 so the Scala 2.12 compilation fails.

You can test this locally by setting the version of Scala in SBT shell with ++2.12.17, eg.

$ sbt
> ++2.12.17
> svsim / compile

GeorgeLyon · 2023-03-21T01:41:36Z

Allowing _ in literals was only added in Scala 2.13 so the Scala 2.12 compilation fails.

Is Scala 2.12 something we care about for Chisel5? (Happy to fix the issue, but just asking)

jackkoenig · 2023-03-21T02:05:31Z

Is Scala 2.12 something we care about for Chisel5? (Happy to fix the issue, but just asking)

Yes, we're deprecating it in Chisel 5 but not removing till Chisel 6.

jackkoenig

Looks good to me (having also reviewed it before 🙂)!

GeorgeLyon · 2023-03-21T21:57:13Z

Thanks @jackkoenig! Just to be clear, are you approving this to be merged or do you want someone else to also take a look?

sequencer

Finally we reached a clean backend!
I just added some nitpick based on my experience. In the future we should consider these:

provide svsim as a library, expose API with mainargs, and can be reused by different build system, including make, mill, wake.
think about how to link the design patterns that consumes other libraries, e.g. spike, we need this for RISC-V cosim or BFM.
provide a API to peek/poke/force internal signals, so that we can do fault injection and dynamic coverage.

sequencer · 2023-03-22T04:38:30Z