Add simple API for generating testharnesses inline #4629

tmckay-sifive · 2025-01-16T21:15:13Z

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

Add an API to generate testharnesses inline that are emitted as additional public modules in the output.

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), 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.

tmckay-sifive · 2025-01-17T20:11:18Z

src/main/scala/chisel3/experimental/HasTests.scala

+    *
+    *  - A clock port
+    *  - A reset port with this module's reset type, or synchronous if unspecified
+    *  - The [[desiredName]] is "[[this.desiredName]] _ [[name]]".


Suggested change

* - The [[desiredName]] is "[[this.desiredName]] _ [[name]]".

* - The [[desiredName]] is "[[this.desiredName]] _ [[testName]]".

src/main/scala/chisel3/experimental/HasTests.scala

seldridge

This took me a while to realize, but this is actually doing a separation of the "test harness" (the body function) from the actual "test". This three-part factoring of (1) DUT, (2) test harness, and (3) test makes sense to me.

With this, have you thought about how to share test harnesses as opposed to having them implicitly inlined in the test? Put differently, is there a way to make the test harness also a module and thereby get the benefits of using D/I on the test harness as opposed to just the DUT?

seldridge · 2025-01-20T20:01:25Z

src/main/scala/chisel3/experimental/HasTests.scala

+/** Provides methods to elaborate additional parents to the circuit. */
+trait ElaboratesParents { module: RawModule =>
+  private lazy val moduleDefinition =
+    module.toDefinition.asInstanceOf[Definition[module.type]]
+
+  /** Generate an additional parent around this module.
+    *
+    *  @param parent generator function, should instantiate the [[Definition]]
+    */
+  def elaborateParentModule(parent: Definition[module.type] => RawModule with Public): Unit =
+    afterModuleBuilt { Definition(parent(moduleDefinition)) }
+}


Can this trait be inlined into its single extension trait, HasTestWithResult?

Alternatively, this could be flattened in the other direction by making this a method on RawModule.

Pre-emptively creating inheritance hierarchies is usually an anti-pattern. (Or: more strongly, inheritance hierarchies are usually an anti-pattern _unless they are architected and/or have some theoretical basis like Scala collections or Cats [1].)

Yeah it could be flattened. The use case I had in mind that lead to this separation was that the user might have a component with a bunch of tests, while still wanting to elaborate a one-off parent module that is not the same as the testharnesses for all the tests.

So, I wanted to have an open-ended elaborateParentModule available to the user for that purpose. But, it felt strange to have that in HasTests, since it's really a separate feature from unit tests; granted one that we can leverage to implement unit tests.

That's how I got here. That said, I'm not very opinionated on the organization. So, if you think it's better Scala or more Chisel-like to flatten these traits, I will happily do so.

seldridge · 2025-01-20T20:47:20Z

src/main/scala/chisel3/experimental/HasTests.scala

+    testName:   String,
+    definition: Definition[module.type],
+    body:       Instance[module.type] => TestResult
+  ): RawModule with Public = new Module with Public {


A Module with Public being immediately cast to a RawModule with Public seems odd.

The intent here is to allow the implementer to override it with something that generates a RawModule.

tmckay-sifive · 2025-01-21T15:47:25Z

With this, have you thought about how to share test harnesses as opposed to having them implicitly inlined in the test? Put differently, is there a way to make the test harness also a module and thereby get the benefits of using D/I on the test harness as opposed to just the DUT?

Good idea, I will look into that.

tmckay-sifive · 2025-01-21T18:07:15Z

With this, have you thought about how to share test harnesses as opposed to having them implicitly inlined in the test? Put differently, is there a way to make the test harness also a module and thereby get the benefits of using D/I on the test harness as opposed to just the DUT?

I think this would require the ability to make a Definition of the testharness, with a hole in the middle for the test body. Is this possible with Definition/Instance? I could make the testharness and test-body/DUT sibling modules to accomplish something similar, but I think it's important that the DUT instance and test RTL live underneath the testharness hierarchy.

jackkoenig · 2025-01-22T22:17:05Z

I have feedback similar to Schuyler's. I think it's really important that we separate the definitions of TestHarnesses from the body of modules that need to be tested. Related to this, I think the type of the TestHarness (and of the TestResult) should not be set for a given module. It seems likely that a single module would want to have tests that have different testharnesses and results. Really, the type of the result should probably be tied to the type of the TestHarness, I don't think they're independent parameters.

I think we can do this with typeclasses, but perhaps we should sit down together to work through it (hard to do in a quick sketch).

A second order concern (and non-blocking)--I think it is useful to be able to create little tests inline so no issues there, but it should not be the only way. We need to think about test composition where I could define a testharness, write a test against the testharness and some DUT interface, and then have multiple different modules use that test for themselves.

Another non-blocking concern, it should be easy to include or elide tests written this way. I should be able to elaborate a module and not also be required to elaborate all of the tests. That can come later though.

jackkoenig · 2025-01-28T01:07:56Z

src/main/scala/chisel3/experimental/InlineTest.scala

+package chisel3.experimental
+
+import chisel3._
+import chisel3.experimental.hierarchy.{Definition, Instance}
+
+package object inlinetest {


Style note, package objects should only be used for things that must be in an object but you want to be accessible within what is otherwise a package. Every top-level definition here is a regular class, trait or object, so there's no reason to use a package object.

Also, the file structure needs to match the package structure.

So basically, please move inlinetest up to the package, and put this code in src/main/scala/chisel3/experimental/inlinetest/...

Gotcha, thanks. I'm not 100% I did what you asked, but I gave it a shot in 5e5bdd4

jackkoenig

Generally this looks great, a few points to consider.

jackkoenig · 2025-01-30T19:36:28Z

src/main/scala/chisel3/experimental/inlinetest/package.scala

@@ -0,0 +1,80 @@
+package chisel3.experimental.inlinetest


Nit, the name of this file shouldn't be package.scala, that is reserved for package objects, e.g. [1].

I'm not sure what to call this file since it has basically everything in the package in it, maybe just InlineTest.scala (still in directory src/main/scala/chisel3/experimental/inlinetest/) and we can break it up later if we see fit?

jackkoenig · 2025-01-30T19:40:26Z

src/main/scala/chisel3/experimental/inlinetest/package.scala

+trait TestHarness[M <: RawModule, R] {
+
+  /** Generate a testharness module given the test parameters. */
+  def generate(test: TestParameters[M, R]): RawModule with Public
+}


Is the type of M every really used? Or put another way--what is the use case for a TestHarness that only works for a particular kind of DUT Module? I suspect there is one, but maybe it would be good to have an example in the tests to drive the point home.

Yep I think it's useful to have when you want to reuse a testharness across DUTs where your testharness expects certain interfaces to be present on the DUT. Added an example.

jackkoenig · 2025-01-30T19:41:57Z

src/main/scala/chisel3/experimental/inlinetest/package.scala

+case class TestParameters[M <: RawModule, R](
+  /** The [[desiredName]] of the DUT module. */
+  dutName: String,
+  /** The user-provided name of the test. */
+  testName: String,
+  /** A Definition of the DUT module. */
+  dutDefinition: Definition[M],
+  /** The body for this test, returns a result. */
+  body: Instance[M] => R
+)


This is an annoying detail but one we have to consider as library writers in Chisel.

What are the odds that we are going to have to evolve this type over time, like adding fields? I'm assuming it's high--if so, we should not make this a case class because case classes are nightmares for both source and binary compatibility.

Also, should users be able to create instances of this object or not? Maybe the constructor, factory apply, and copy methods should all be private (we won't have the latter two if we make this not a case class).

What are the odds that we are going to have to evolve this type over time, like adding fields? I'm assuming it's high--

Yeah, I would expect we will at some point.

if so, we should not make this a case class because case classes are nightmares for both source and binary compatibility.

Good to know. I made it a case class precisely because I wanted to avoid changes to the parameter list of the methods that take them.

I've changed it to a class for now.

jackkoenig · 2025-01-30T19:43:58Z

src/main/scala/chisel3/experimental/inlinetest/package.scala

+  *  @tparam M the type of the DUT module
+  *  @tparam R the type of the result returned by the test body
+  */
+trait TestHarness[M <: RawModule, R] {


Another nit but something to think about. Neither UnitTestHarness nor TestHarnessWithResultIO extend this trait which leads me to believe TestHarness is the wrong name for this TestHarness-providing-typeclass. I am not sure of the right name though. TestHarnessGenerator? @seldridge any ideas?

Yeah I think TestHarnessGenerator makes more sense. I've changed it to that pending other opinions.

tmckay-sifive · 2025-01-31T04:51:59Z

Incidentally, the extra logic I added to the tests now either produces invalid FIR or hits a bug in firtool.

FIRRTL version 4.2.0
circuit test_ModuleWithTests_with_monitor :
  layer Verification, bind, "verification" :
    layer Assert, bind, "verification/assert" :
    layer Assume, bind, "verification/assume" :
    layer Cover, bind, "verification/cover" :

  module ModuleWithTests :
    input clock : Clock
    input reset : UInt<1>
    output io : { flip in : UInt<32>, out : UInt<32>}
    output monProbe : Probe<{ in : UInt<32>, out : UInt<32>}>

    define monProbe = probe(io)
    connect io.out, io.in

  module ProtocolMonitor :
    input clock : Clock
    input reset : Reset
    input io : { in : UInt<32>, out : UInt<32>}

    node _T = eq(io.in, io.out)
    node _T_1 = asUInt(reset)
    node _T_2 = eq(_T_1, UInt<1>(0h0))
    layerblock Verification :
      layerblock Assert :
        intrinsic(circt_chisel_ifelsefatal<format = "Assertion failed: in === out\n", label = "chisel3_builtin">, clock, _T, _T_2)

  public module test_ModuleWithTests_with_monitor :
    input clock : Clock
    input reset : UInt<1>

    inst dut of ModuleWithTests
    connect dut.clock, clock
    connect dut.reset, reset
    connect dut.io.in, UInt<32>(0h5)
    node _T = neq(dut.io.out, UInt<1>(0h0))
    node _T_1 = eq(reset, UInt<1>(0h0))
    layerblock Verification :
      layerblock Assert :
        intrinsic(circt_chisel_ifelsefatal<format = "Assertion failed at InlineTestSpec.scala:110\n", label = "chisel3_builtin">, clock, _T, _T_1)

    inst monitor of ProtocolMonitor
    connect monitor.clock, clock
    connect monitor.reset, reset
    connect monitor.io.out, read(dut.monProbe).out
    connect monitor.io.in, read(dut.monProbe).in

  circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /sifive/tools/llvm/circt/1.103.0/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+306-941cc4b7-SNAPSHOT) was published against firtool version 1.100.0.
------------------------------------------------------------------------------
ExitCode:
1
STDOUT:

STDERR:
src/test/scala/chisel3/experimental/InlineTestSpec.scala:20:23: error: 'firrtl.instance' op has a wrong number of results; expected 2 but got 4
    val dut = Instance(test.dutDefinition)
                      ^
src/test/scala/chisel3/experimental/InlineTestSpec.scala:20:23: note: see current operation: %5:4 = "firrtl.instance"() <{annotations = [], layers = [], moduleName = @ModuleWithTests, name = "dut", nameKind = #firrtl<name_kind droppable_name>, portAnnotations = [[], [], [], []], portDirections = array<i1: false, true, true, true>, portNames = ["io_in", "io_out", "monProbe_in", "monProbe_out"]}> : () -> (!firrtl.uint<32>, !firrtl.uint<32>, !firrtl.probe<uint<32>>, !firrtl.probe<uint<32>>)
src/test/scala/chisel3/experimental/InlineTestSpec.scala:69:2: note: original module declared here
@instantiable
 ^

Not sure which yet, need to look into it.

This reverts commit 3b24f28.

tmckay-sifive · 2025-01-31T18:35:22Z

Not sure which yet, need to look into it.

Turns out CIRCT bug. I've uncommented the test so it will continue to fail until fixed. Might need to rewrite the test to circumvent the bug if it's not fixed soon.

seldridge · 2025-02-05T03:38:54Z

@tmckay-sifive: FYI, this is fixed in CIRCT 1.104.0 and I'm seeing that test do something on my end.

seldridge

Some meandering feedback. I'm primarily looking to see if there is a way to avoid the boilerplate of the test harnesses around resetType, desired name, and the dut instantiation.

I think the main thing to consider is where this is highlighting what we are missing on the FIRRTL side in order to run these tests. We probably want to have a defined "test interface" off of which everything works. This PR isn't incompatible with that in any way.

src/main/scala/chisel3/experimental/inlinetest/InlineTest.scala

src/test/scala/chisel3/experimental/InlineTestSpec.scala

src/main/scala/chisel3/experimental/inlinetest/InlineTest.scala

src/test/scala/chisel3/experimental/InlineTestSpec.scala

seldridge · 2025-02-05T03:59:28Z

src/main/scala/chisel3/experimental/inlinetest/InlineTest.scala

+class TestParameters[M <: RawModule, R] private[inlinetest] (
+  /** The [[desiredName]] of the DUT module. */
+  val dutName: String,
+  /** The user-provided name of the test. */
+  val testName: String,
+  /** A Definition of the DUT module. */
+  val dutDefinition: Definition[M],
+  /** The body for this test, returns a result. */
+  val body: Instance[M] => R
+)


This is almost a case class. Any reason to not make it one?

I originally had it that way, but according to @jackkoenig that is a problem for binary compatibility. I don't fully understand the reason for that, I'll admit.

src/main/scala/chisel3/experimental/inlinetest/InlineTest.scala

src/test/scala/chisel3/experimental/InlineTestSpec.scala

seldridge · 2025-02-05T17:38:32Z

src/main/scala/chisel3/experimental/inlinetest/InlineTest.scala

+  /** The minimal implementation of a unit testharness. Has a clock input and a synchronous reset
+    *  input. Connects these to the DUT and does nothing else.
+    */


Is clock and reset sufficient? @fabianschuiki has brought up some points about: (1) knowing whether or not the test is done and (2) knowing what the exit status is.

Does this motivate a moderately more heavyweight interface of:

class UnitTestBundle extends Bundle { val clock = Flipped(Clock()) val reset = Flipped(Bool()) val done = Bool() val exitCode = UInt(32.W) }

Alternatively, maybe there are two kinds of tests:

Tests that finish with either an assertion failure or a stop.

Tests that finish using the done/exitCode

Note: if the stop is dropped from (1), then (1) becomes purely a subset of (2).

Yes great point. This was my main motivation for making it easy for users to override the testharness generator and result from each test. I imagine each project might have their own contract with a test driver to communicate this information. I agree that done/exitCode is the obvious way to do that and more or less required for a sensible unit testing setup.

Since I do think most projects will settle on some testharness-testdriver contract to communicate done/pass, think it comes down to whether we prefer to ship an example implementation of this by default, or expect users to implement their own if they want it.

I landed on not adding done/pass because it felt less "presumptuous" of the users simulation environment. But I'm open to arguments that that comes at the cost of usability.

The reason why I'm leaning towards it is that we want to have the ability to do both test-discovery and test-execution for any test defined this way. The part which is missing is that we can do test-discovery with some small changes to FIRRTL to add a simulate statement/region similar to how we have a formal statement/region (see: https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#formal-unit-tests). Essentially, these tests just need a marker to indicate that they are tests and not just modules.

However, while formal has enough information to tell it also how to run the test (because a formal test is both simpler and the formal construct is powerful enough to capture stimulus via assume), simulate does not have enough information to run the test. Even in the simplest formulation of just having clock/reset, there are assumptions about how to drive the clock and the reset.

It's kind of like defining an entry point for a program (C/C++'s main function void main(int argc, char ** argv) (or the equivalent in other languages) comes to mind as does function calling convention).

FWIW, stop does include an exit code, so perhaps that is actually sufficient to use with only the clock/reset formulation. That actually sounds good.

Makes sense to me. If this is not blocking feedback, I will leave it for now.

I think it is a reasonable expectation that these testharness modules are not the top when we actually compile the simulation--there will be a test-driver layer above that generating clocks and resets, writing waves, etc. So, I think what we are looking for is an ABI between the test-driver and the testharness.

Perhaps testharnesses that can be simulated must implement this ABI. At which point, circt-test could have its own intrinsic test-driver that works well with verilator and has sane/configurable logic for clock/reset generation.

seldridge

This looks great! Thank you for driving this!

We will need to sort out how to get the test-discovery and test-execution parts of this working. However, this is a great foundation on top of which we can start. 👍

src/main/scala/chisel3/experimental/inlinetest/InlineTest.scala

tmckay-sifive added 4 commits January 16, 2025 13:14

add HasTests

db99d18

reformat

d96675c

make it easier to override testharness generation

4ffc80c

add scaladoc

c9b887b

tmckay-sifive commented Jan 17, 2025

View reviewed changes

src/main/scala/chisel3/experimental/HasTests.scala Outdated Show resolved Hide resolved

tmckay-sifive marked this pull request as ready for review January 17, 2025 20:15

reformat

80f5a31

seldridge added the Feature New feature, will be included in release notes label Jan 20, 2025

seldridge reviewed Jan 20, 2025

View reviewed changes

flatten traits

90f68a6

tmckay-sifive requested a review from seldridge January 22, 2025 17:18

refactor based on PR feedback

c18e660

jackkoenig reviewed Jan 28, 2025

View reviewed changes

tmckay-sifive added 2 commits January 27, 2025 19:11

reformat

a46103d

fix inlinetest package organization

5e5bdd4

tmckay-sifive force-pushed the tmckay/inline-testing branch from 783b9d6 to 5e5bdd4 Compare January 28, 2025 03:12

tmckay-sifive requested a review from jackkoenig January 30, 2025 17:30

jackkoenig reviewed Jan 30, 2025

View reviewed changes

tmckay-sifive added 7 commits January 30, 2025 19:21

rename to InlineTest.scala

60f2e5f

make TestParameters not a case class

a1a1a7f

make TestParameters private[inlinetest]

502e0e9

rename TestHarness to TestHarnessGenerator

1436ea2

add example that utilizes DUT type in testharness

d8aaaa7

REVERTME: comment out failing test

3b24f28

scala format

941cc4b

tmckay-sifive added 2 commits January 31, 2025 10:33

Revert "REVERTME: comment out failing test"

0488111

This reverts commit 3b24f28.

update test

2e715dd

Merge remote-tracking branch 'origin/main' into tmckay/inline-testing

77291c9

tmckay-sifive requested a review from jackkoenig February 4, 2025 17:25

seldridge reviewed Feb 5, 2025

View reviewed changes

tmckay-sifive added 7 commits February 5, 2025 09:46

add license string

81202f6

remove extra space

92e208b

update tests per feedback

abe0c6b

move test: src/test/scala/chisel3 -> src/test/scala/chiselTests

f7b5887

misc feedback

6cf7428

add TestHarnessRawModule and TestHarnessModule traits

70b5b40

reformat

659adc9

seldridge approved these changes Feb 5, 2025

View reviewed changes

tmckay-sifive added 2 commits February 5, 2025 15:47

PR feedback

6760320

Merge branch 'main' into tmckay/inline-testing

fb52653

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple API for generating testharnesses inline #4629

Add simple API for generating testharnesses inline #4629

tmckay-sifive commented Jan 16, 2025 •

edited

Loading

tmckay-sifive Jan 17, 2025

seldridge left a comment

seldridge Jan 20, 2025

tmckay-sifive Jan 21, 2025

seldridge Jan 20, 2025

tmckay-sifive Jan 21, 2025

tmckay-sifive commented Jan 21, 2025

tmckay-sifive commented Jan 21, 2025

jackkoenig commented Jan 22, 2025 •

edited

Loading

jackkoenig Jan 28, 2025

tmckay-sifive Jan 28, 2025

jackkoenig left a comment

jackkoenig Jan 30, 2025

jackkoenig Jan 30, 2025

tmckay-sifive Jan 31, 2025

jackkoenig Jan 30, 2025

tmckay-sifive Jan 31, 2025

jackkoenig Jan 30, 2025

tmckay-sifive Jan 31, 2025

tmckay-sifive commented Jan 31, 2025

tmckay-sifive commented Jan 31, 2025

seldridge commented Feb 5, 2025

seldridge left a comment

seldridge Feb 5, 2025

tmckay-sifive Feb 5, 2025

seldridge Feb 5, 2025

tmckay-sifive Feb 5, 2025 •

edited

Loading

seldridge Feb 5, 2025

tmckay-sifive Feb 5, 2025 •

edited

Loading

seldridge left a comment

	* - The [[desiredName]] is "[[this.desiredName]] _ [[name]]".
	* - The [[desiredName]] is "[[this.desiredName]] _ [[testName]]".

Add simple API for generating testharnesses inline #4629

Are you sure you want to change the base?

Add simple API for generating testharnesses inline #4629

Conversation

tmckay-sifive commented Jan 16, 2025 • edited Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Choose a reason for hiding this comment

seldridge left a 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

tmckay-sifive commented Jan 21, 2025

tmckay-sifive commented Jan 21, 2025

jackkoenig commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkoenig left a 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

tmckay-sifive commented Jan 31, 2025

tmckay-sifive commented Jan 31, 2025

seldridge commented Feb 5, 2025

seldridge left a 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

tmckay-sifive Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmckay-sifive Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

tmckay-sifive commented Jan 16, 2025 •

edited

Loading

jackkoenig commented Jan 22, 2025 •

edited

Loading

tmckay-sifive Feb 5, 2025 •

edited

Loading

tmckay-sifive Feb 5, 2025 •

edited

Loading