-
Notifications
You must be signed in to change notification settings - Fork 623
ChiselSim integration for inline tests #4855
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
base: main
Are you sure you want to change the base?
ChiselSim integration for inline tests #4855
Conversation
Depends on these PRs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool!
I started to review this then realized there are a bunch of prerequisite PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to try to avoid the proliferation of different functions that all have slightly different arguments. This was, IMO, a big mistake in both our external and internal testing flows.
What I would do is move in a direction of adding a parameter with a default value that gives you a type-safe way to choose the tests, e.g., an enum like:
object TestSelection {
sealed trait Type
case object All
case class Exactly(tests: Seq[String])
case class Glob(glob: String)
}
I'm not proposing the above organization exactly, just something like this. You could see LayerControl
for an example of how to organize this.
firtoolOptsModifications: FirtoolOptionsModifications, | ||
commonSettingsModifications: svsim.CommonSettingsModifications, | ||
backendSettingsModifications: svsim.BackendSettingsModifications | ||
): Seq[(String, Simulator.BackendInvocationDigest[_])] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type of these methods is too low-level. This should just be Unit
. Alternatively, there should be a ChiselSim
-specific return type that is used for all methods here. I.e., also for simulate
and simulateRaw
.
0f604d3
to
25182fb
Compare
e1c4ba7
to
8879da5
Compare
This has been cleaned up and is ready for review. Note: this branch is based on #4833. EDIT: For reviewers, probably best to hold off for a bit longer. I put this in draft. I have some cleanups I want to backport to this branch that I made in order to support generating scalatest specs for inline tests. |
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
ChiselSim.simulateTests
API to simulate inline tests and report resultsReviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending on impact, API modification or big change:7.0
)?Enable auto-merge (squash)
and clean up the commit message.Create a merge commit
.