Towards an ergonomic chiselsim testing framework [svsim] [RFC] [WIP] #4209
Conversation
|
I've listed a couple of improvements in #4203 that would make testing with ChiselSim closer to what chiseltest is which is very user-friendly IMO. |
|
This looks great! Thanks for taking the lead on this @kammoh! Please feel free to re-use code from chiseltest and tag me if you have any questions. The only thing you need to be careful about is to maintain the correct license information. Most of my contributions to chiseltest were licensed under BSD 3-Clause License. You will see that noted in the file header, just make sure to copy that information over with any code snippets. |
There was a problem hiding this comment.
Thanks so much for getting this going @kammoh. Some drive by comments but the biggest one is that this is awesome!
build.sbt
Outdated
| @@ -195,7 +195,7 @@ lazy val firrtl = (project in file("firrtl")) | |||
| lazy val chiselSettings = Seq( | |||
| name := "chisel", | |||
| libraryDependencies ++= Seq( | |||
| "org.scalatest" %% "scalatest" % "3.2.18" % "test", | |||
| "org.scalatest" %% "scalatest" % "3.2.18", | |||
There was a problem hiding this comment.
We should not impose scalatest on downstream users of Chisel. Better options IMO are:
- Add a new subproject that adds just the integration (e.g.
chisel-scalatest) - the rest of chiselsim stays in chisel but the sugar around scalatest specs lives in the new project. - Make it a
provideddependency - Use runtime reflection
(1) is a little annoying but there are no sharp edges other than telling the user to grab the dependency. (2) means that if users forget to add the dependency but then use the APIs requiring chiseltest, they will get a linkage error. We can mitigate that by catching the exception and giving a better error, or perhaps just providing a sensible default. Regardless, there are more sharp edges. (3) is similar to (2) except Chisel itself could elide the dependency. I don't think there are any benefits to it over (2) and it has all of the same sharp edges.
| def waitForValid() = { | ||
| // println("wait for valid") | ||
| // clock.stepUntil(sig.valid, 1, maxWaitCycles) | ||
| // val timeout = sig.valid.peekValue().asBigInt == 0 | ||
| // chisel3.assert(!timeout, s"Timeout after $maxWaitCycles cycles waiting for valid") | ||
| var cycles = 0 | ||
| while (!sig.valid.peek().litToBoolean) { | ||
| clock.step() |
There was a problem hiding this comment.
This would be a great place to take advantage of SVSims clock ticking with sentinel values:
chisel/svsim/src/test/scala/BackendSpec.scala
Line 151 in 2fdaead
| @@ -289,7 +289,7 @@ object Flipped { | |||
| * @groupdesc Connect Utilities for connecting hardware components | |||
| * @define coll data | |||
| */ | |||
| abstract class Data extends HasId with NamedComponent with SourceInfoDoc { | |||
| abstract class Data extends HasId with NamedComponent with SourceInfoDoc with Serializable { | |||
There was a problem hiding this comment.
Whats the Serializable for?
| * @param chiselArgs | ||
| * @param firtoolArgs | ||
| */ | ||
| case class ChiselSimSettings[B <: Backend]( |
There was a problem hiding this comment.
I don't think we should make this a case class. They are so much more convenient to write but they make adding fields in a backwards compatible way very painful (generated unapply and copy methods are the bane of source and binary compatibility). I assume we're going to want to add settings to this quite a bit, so we should probably set ourselves up to make that easy.
| outputSplit: Option[Int] = Some(20_000), | ||
| outputSplitCFuncs: Option[Int] = Some(20_000), |
There was a problem hiding this comment.
I think we should avoid making simualtor-specific things part of the generic ChiselSim settings
| import svsim.Simulation | ||
|
|
||
| trait HierarchicalValue { | ||
| val gen: Data |
There was a problem hiding this comment.
| val gen: Data | |
| def gen: Data |
Generally, it's best to make any virtual methods a def and let implementers decide what to do
|
Hi, thanks for your effort. I want to know whether SVSIM supports multiple clocks now? |
|
This is VERY cool! Hope to have a smooth transition from classic ChiselTest to the new simulator. |
kammoh
force-pushed
the
ChiselSimTester
branch
from
d0e9100
to
953e62f
Compare
This is an attempt towards a full-featured and user-friendly testing framework with
chiseltestcapabilities. I really like the chiseltest's API @ekiwi and ergonomics and I think keeping a similar interface would greatly ease transition and porting to svsim.Notable missing features include:
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.