This adds a mechanism for the unittests to be run with the TreadleBac… #1483

chick · 2020-06-19T18:30:38Z

This PR introduces a dependency on Treadle for backend testing.
Looking for comments on implementation and concept.

Type of change: Testing enhancement

Impact: no functional change to chisel
This could enable running unit tests to go much faster, probably about 10x.
This also helps make sure Treadle has a high fidelity to verilator.

Development Phase: implementation

Release Notes
Chisel test suite tests can now be run using Treadle (manually).
This PR presents a use case for the Issue on global settings

Release Notes

The is the first part of a facility for running the chisel tests much faster using the Treadle backend.
Verilator remains the standard backend.

ducky64

Architecturally looks fine, but there seems to be some (possibly unintentional?) change of test behavior. Also suggestions for improvements (including documentation), based on my understanding of what the code does.

src/main/scala/chisel3/testers/TesterDriver.scala

ducky64 · 2020-06-19T19:53:01Z

src/main/scala/chisel3/testers/TesterDriver.scala

+        executeTreadle(t, additionalVResources, annotations, nameHint)
+      case VerilatorBackend =>
+        executeVerilog(t, additionalVResources, annotations, nameHint)
+      case _ => true


One (possible?) edge case is if there are multiple backend annotations, it will run once per annotation.
Also, potentially fragile in that it's possible to define additional backend classes which could be silently dropped (as opposed to failing noisily) here.

A potentially cleaner / more robust way to write this function would be filtering annotations for type of Backend, optionally checking the list is one or zero elements, getting the first element, and matching on it. Since it's guaranteed to be of type Backend, the match can be complete and at least fail noisily (possibly even statically checked) if a condition is missed.

This hints at TesterDriver needing a standard stage/phase refactor. This type of check would normally be handled with a Checks phase.

@ducky64 Made code require a single known backend to be used.
@seldridge Yes, that would probably be better. I'd prefer not to implement now, but would like to discuss details with you

ducky64 · 2020-06-19T20:01:58Z

src/main/scala/chisel3/testers/TesterDriver.scala

+//  val defaultBackend: Backend = VerilatorBackend
+  val defaultBackend: Backend = TreadleBackend
+
+  /** Use this to force a test to be run only with backends that are not Treadle


Might be more succinctly described as skipping tests when not running with Verilator.

ducky64 · 2020-06-19T20:02:40Z

src/main/scala/chisel3/testers/TesterDriver.scala

+              additionalVResources: Seq[String] = Seq(),
+              annotations:          AnnotationSeq = Seq(),
+              nameHint:             Option[String] = None): Boolean = {
+


style nit: unnecessary whitespace lines?

ducky64 · 2020-06-19T21:05:00Z

src/main/scala/chisel3/testers/TesterDriver.scala

+
+    val circuit = annotationSeq.collect { case x: ChiselCircuitAnnotation => x }.head.circuit
+
+//    val targetName = s"test_run_dir/${circuit.name}" + (nameHint match {


ducky64 · 2020-06-19T21:05:11Z

src/main/scala/chisel3/testers/TesterDriver.scala

+    val targetName: File = createTestDirectory(circuit.name)
+
+    annotationSeq = annotationSeq :+ TargetDirAnnotation(targetName.getPath) // :+ ClockInfoAnnotation(Seq(ClockInfo("clock", 10, 0))) // :+ CallResetAtStartupAnnotation
+    annotationSeq = annotationSeq :+ TargetDirAnnotation(targetName.getPath) :+ CallResetAtStartupAnnotation


is the duplication of TargetDirAnnotation intentional?

ducky64 · 2020-06-19T21:12:08Z

src/test/scala/chiselTests/BlackBox.scala

@@ -150,23 +150,28 @@ class BlackBoxWithParamsTester extends BasicTester {
 class BlackBoxSpec extends ChiselFlatSpec {
  "A BlackBoxed inverter" should "work" in {
    assertTesterPasses({ new BlackBoxTester },
-        Seq("/chisel3/BlackBoxTest.v"))
+        Seq("/chisel3/BlackBoxTest.v"),
+      annotations = TesterDriver.verilatorOnly)


style nit: any reason this line isn't aligned with the above?

Also, in some cases, you use annotations = ..., and in others, you don't have the explicit keyword. Is this by choice (and what's the underlying rationale) or chance?

I just followed the previous alignment here. But it is pretty messed up so I have reformatted.
Removed using named parameter

ducky64 · 2020-06-19T21:12:52Z

src/test/scala/chiselTests/Counter.scala

@@ -41,7 +44,11 @@ class CounterSpec extends ChiselPropSpec {
  }

  property("Counter can be en/disabled") {
-    forAll(safeUInts) { (seed: Int) => whenever(seed >= 0) { assertTesterPasses{ new EnableTester(seed) } } }
+    assertTesterPasses{ new EnableTester(4) }
+//    forAll(safeUInts) { (seed: Int) => whenever(seed >= 0) {


Dead code?
Wait, this actually removes the forall behavior?

Good catch, debugging code that slipped through

ducky64 · 2020-06-19T21:14:13Z

src/test/scala/chiselTests/Counter.scala

@@ -22,6 +22,9 @@ class EnableTester(seed: Int) extends BasicTester {
  val (_, done) = Counter(true.B, 33)

  when(done) {
+    when(! cntEnVal === popCount(seed).asUInt) {


prints in automated regressions are probably generally discouraged?

ducky64 · 2020-06-19T21:15:20Z

.gitignore

@@ -12,3 +12,4 @@ test_run_dir
 *~
 \#*\#
 .\#*
+TestBackendName


seldridge · 2020-06-19T21:57:26Z

It may make sense to sneak #1481 in before this (which I was intending to backport to 3.3.x). That cleans up the TesterDriver.execute method to be more stage-like and may help simplify the logic of switching backends.

ducky64

lgtm

Two (optional) suggestions (including the whitespace line after TesterDriver.execute, which looks like it may have been partially addressed) if you feel like it.

ducky64 · 2020-06-20T19:28:38Z

src/main/scala/chisel3/testers/TesterDriver.scala

+              nameHint:             Option[String] = None): Boolean = {
+
+    val backendAnnotations = annotations.collect { case anno: Backend => anno }
+    val backendAnnotation = if (backendAnnotations.length == 1) {


Random thought: would it be cleaner (or possible) to do a list match? - take the filtered / collected list and match against single-element list/seq/etc cases?

@ducky64

…kend This mechanism is not enabled and should not change the behavior of existing tests A following PR will deliver a switch that will allow changing the backend. The reasons for this PR - Treadle tests run much faster, enabling quicker debugging and CI cycles - This will help ensure fidelity of Treadle to the Verilator backend A few tests are marked as verilator only due to black box limitations Change treadle to a direct dependency I tried to make it a test only dependency but the TesterDriver sits in src/main requiring that regular compile have access to treadle Oops, made treadle the default A number of changes in response to @ducky64 review - made backend check clearer and add error handling for multiple backends specified - Fixed duplicate TargetDirAnnotation uses in Treadle backend - Cleaned up BlackBox test formatting - Undid unnecessary debugging changes from Counter - Undid .gitignore change, that should be on another PR A number of changes in response to @ducky64 review - Undid debugging changes made to BitWiseOps

chick added feature request Request For Comment labels Jun 19, 2020

chick added this to the 3.4.0 milestone Jun 19, 2020

chick requested review from ducky64 and jackkoenig June 19, 2020 18:30

chick requested a review from a team as a code owner June 19, 2020 18:30

chick self-assigned this Jun 19, 2020

chick mentioned this pull request Jun 19, 2020

Run chisel test suite with Treadle #1279

Closed

ducky64 requested changes Jun 19, 2020

View reviewed changes

ducky64 approved these changes Jun 20, 2020

View reviewed changes

chick added 2 commits June 29, 2020 10:22

- A few final fixes after the rebase

3694b09

chick force-pushed the add-treadle-backend-for-tests branch from 9f3192d to 3694b09 Compare June 29, 2020 17:44

chick merged commit 61f4abd into master Jun 30, 2020

sequencer mentioned this pull request Jul 2, 2020

fix treadle dependency for mill #1504

Merged

jackkoenig deleted the add-treadle-backend-for-tests branch July 7, 2021 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This adds a mechanism for the unittests to be run with the TreadleBac… #1483

This adds a mechanism for the unittests to be run with the TreadleBac… #1483

chick commented Jun 19, 2020

ducky64 left a comment

ducky64 Jun 19, 2020

seldridge Jun 19, 2020

chick Jun 20, 2020 •

edited

Loading

ducky64 Jun 19, 2020

chick Jun 20, 2020

ducky64 Jun 19, 2020

ducky64 Jun 19, 2020

chick Jun 20, 2020

ducky64 Jun 19, 2020

chick Jun 20, 2020

ducky64 Jun 19, 2020

ducky64 Jun 19, 2020

chick Jun 20, 2020

ducky64 Jun 19, 2020

chick Jun 20, 2020

ducky64 Jun 19, 2020

ducky64 Jun 19, 2020

chick Jun 20, 2020

seldridge commented Jun 19, 2020

ducky64 left a comment

ducky64 Jun 20, 2020


		val circuit = annotationSeq.collect { case x: ChiselCircuitAnnotation => x }.head.circuit

		// val targetName = s"test_run_dir/${circuit.name}" + (nameHint match {

This adds a mechanism for the unittests to be run with the TreadleBac… #1483

This adds a mechanism for the unittests to be run with the TreadleBac… #1483

Conversation

chick commented Jun 19, 2020

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge commented Jun 19, 2020

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick Jun 20, 2020 •

edited

Loading